[PATCH] scripts/modpost: check for bad refernces in .pci.fixups area

[PATCH] scripts/modpost: check for bad refernces in .pci.fixups area

[Sebastian Andrzej Siewior]

Functions used for PCI fixups (like DECLARE_PCI_FIXUP_HEADER) are often
marked __init. This is okay as long as nobody is using PCI hotplug.
That means if one executes
| echo 1 > /sys/bus/pci/rescan

and we hit a module which is marked __init instead of __devinit then we
go boom because the code is removed after the kernel booted. This patch
helps to see those section miss-matches.
During rescan I saw that pci_fixup_early, pci_fixup_header and
pci_fixup_enable have been called. I added pci_fixup_final (which has no
users) and resume hooks as they should also not remain in __init
section.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 scripts/mod/modpost.c |   11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index c4e7d15..b6fa8be 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -862,6 +862,11 @@ static void check_section(const char *modname, struct elf_info *elf,
 #define ALL_EXIT_TEXT_SECTIONS \
 	".exit.text$", ".devexit.text$", ".cpuexit.text$", ".memexit.text$"
 
+#define ALL_PCI_INIT_SECTIONS	\
+	".pci_fixup_early$", ".pci_fixup_header$", ".pci_fixup_final$", \
+	".pci_fixup_enable$", ".pci_fixup_resume$", \
+	".pci_fixup_resume_early$", ".pci_fixup_suspend$"
+
 #define ALL_XXXINIT_SECTIONS DEV_INIT_SECTIONS, CPU_INIT_SECTIONS, \
 	MEM_INIT_SECTIONS
 #define ALL_XXXEXIT_SECTIONS DEV_EXIT_SECTIONS, CPU_EXIT_SECTIONS, \
@@ -1024,6 +1029,12 @@ const struct sectioncheck sectioncheck[] = {
 	.mismatch = ANY_EXIT_TO_ANY_INIT,
 	.symbol_white_list = { DEFAULT_SYMBOL_WHITE_LIST, NULL },
 },
+{
+	.fromsec = { ALL_PCI_INIT_SECTIONS, NULL },
+	.tosec   = { INIT_SECTIONS, NULL },
+	.mismatch = ANY_INIT_TO_ANY_EXIT,
+	.symbol_white_list = { NULL },
+},
 /* Do not export init/exit functions or data */
 {
 	.fromsec = { "__ksymtab*", NULL },
-- 
1.7.10

Re: [PATCH] scripts/modpost: check for bad refernces in .pci.fixups area

[Bjorn Helgaas]

On Thu, May 3, 2012 at 9:23 AM, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
> Functions used for PCI fixups (like DECLARE_PCI_FIXUP_HEADER) are often
> marked __init. This is okay as long as nobody is using PCI hotplug.
> That means if one executes
> | echo 1 > /sys/bus/pci/rescan
>
> and we hit a module which is marked __init instead of __devinit then we
> go boom because the code is removed after the kernel booted. This patch
> helps to see those section miss-matches.
> During rescan I saw that pci_fixup_early, pci_fixup_header and
> pci_fixup_enable have been called. I added pci_fixup_final (which has no
> users) and resume hooks as they should also not remain in __init
> section.
>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Most of the things this finds are quirks for devices one could argue
are not hot-pluggable, like northbridges, I/O APICs, etc.  But some of
those are becoming hot-pluggable, and I think it's too easy to make
the wrong __init/__devinit decision, so I think the safest thing is to
make them all __devinit.

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

I assume this will be merged via a kbuild tree; let me know if otherwise.

Will you post a follow-up patch to fix the PCI quirk annotations?

>  scripts/mod/modpost.c |   11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index c4e7d15..b6fa8be 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -862,6 +862,11 @@ static void check_section(const char *modname, struct elf_info *elf,
>  #define ALL_EXIT_TEXT_SECTIONS \
>        ".exit.text$", ".devexit.text$", ".cpuexit.text$", ".memexit.text$"
>
> +#define ALL_PCI_INIT_SECTIONS  \
> +       ".pci_fixup_early$", ".pci_fixup_header$", ".pci_fixup_final$", \
> +       ".pci_fixup_enable$", ".pci_fixup_resume$", \
> +       ".pci_fixup_resume_early$", ".pci_fixup_suspend$"
> +
>  #define ALL_XXXINIT_SECTIONS DEV_INIT_SECTIONS, CPU_INIT_SECTIONS, \
>        MEM_INIT_SECTIONS
>  #define ALL_XXXEXIT_SECTIONS DEV_EXIT_SECTIONS, CPU_EXIT_SECTIONS, \
> @@ -1024,6 +1029,12 @@ const struct sectioncheck sectioncheck[] = {
>        .mismatch = ANY_EXIT_TO_ANY_INIT,
>        .symbol_white_list = { DEFAULT_SYMBOL_WHITE_LIST, NULL },
>  },
> +{
> +       .fromsec = { ALL_PCI_INIT_SECTIONS, NULL },
> +       .tosec   = { INIT_SECTIONS, NULL },
> +       .mismatch = ANY_INIT_TO_ANY_EXIT,
> +       .symbol_white_list = { NULL },
> +},
>  /* Do not export init/exit functions or data */
>  {
>        .fromsec = { "__ksymtab*", NULL },
> --
> 1.7.10
>

Re: [PATCH] scripts/modpost: check for bad refernces in .pci.fixups area

[Sebastian Andrzej Siewior]

On 05/03/2012 08:12 PM, Bjorn Helgaas wrote:
> Most of the things this finds are quirks for devices one could argue
> are not hot-pluggable, like northbridges, I/O APICs, etc.  But some of
> those are becoming hot-pluggable, and I think it's too easy to make
> the wrong __init/__devinit decision, so I think the safest thing is to
> make them all __devinit.

I got here via a PCIe-bridge quirk. The bridge itself is not removed
but the quirk is executed after "echo 1 > remove" followed by rescan.

> Acked-by: Bjorn Helgaas<bhelgaas@google.com>
>
> I assume this will be merged via a kbuild tree; let me know if otherwise.

I tried to figured who is in charge here. I guess either Marek or
Alessio Bogani will take it. Atleast I have the PCI-ack here :)

> Will you post a follow-up patch to fix the PCI quirk annotations?

Since you are fine with it, let me try to prepare some.

Sebastian