2.6.0-test6: a few __init bugs

Here are several places where non-__init functions call __init functions or reference __init data. I've looked at all of them and believe that they are all either legitimate bugs or opportunities to declare more code as __init to save memory. Thanks for looking over these, and sorry if I've made any mistakes.

Best,
Rob

P.S. All these bugs were found with Cqual, the bug-finding tool developed by Jeff Foster, John Kodumal, and many others, and available at http://www.cs.umd.edu/~jfoster/cqual/, although the currently released version of cqual only has primitive support for __init bug-finding.

Linux 2.6.0-test6:

** Possible bug:

** drivers/pci/quirks.c:asus_hides_smbus_hostbridge()                  (__init)
   in table: drivers/pci/quirks.c:pci_fixups                           (not __init)
     indirect call f->hook(): drivers/pci/quirks.c:pci_do_fixups()     (not __init)
       called by: drivers/pci/quirks.c:pci_fixup_device()              (not __init)
         called by: drivers/pci/probe.c:pci_scan_slot()                (not __init)

called by lots of hotplug enable() functions, e.g.
drivers/pci/hotplug/ibmphp_core.c:ibm_configure_device() (not __init)
called by drivers/pci/hotplug/ibmphp_core.c:enable_slot() (not __init)

Note: It looks like this may have been originally designed to initialize the
pci bus at startup, but has been re-used in the hotplug code, which means it can be run after the __init segments have gone away.

Fix: Delete all the __init declarations on the quirks hooks.

** Code can be declared __init:
** arch/i386/kernel/cpu/mtrr/generic.c:get_fixed_ranges() (__init)
called by: arch/i386/kernel/cpu/mtrr/generic.c:get_mtrr_state() (not __init)
only caller: arch/i386/kernel/cpu/mtrr/main.c:init_other_cpus() (not __init)
only caller: arch/i386/kernel/cpu/mtrr/main.c:mtrr_init() (__init)
Fix: Declare get_mtrr_state() __init
Fix: Declare init_other_cpus() __init

** Code can be declared __init:
** arch/i386/kernel/io_apic.c:io_apic_set_pci_routing() (not __init)
all callers are __init
Fix: Declare io_apic_set_pci_routing() __init

** Code should be declared __init?

** name_to_dev_t()                                                     (__init)
     called by kernel/power/swsusp.c:read_suspend_image()              (not __init)
       called by kernel/power/swsusp.c:software_resume()               (not __init)

Fix: declare read_suspend_image() __init
Fix: declare software_resume() __init

Note: read_suspend_image() in pmdisk.c is declared __init.

Hi!

Yes, it should be safe to declare that code __init. Applied.

Pavel

When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

No, this was discussed a lot a year or so ago. We don't currently have anything in the quirks table that can be hotplug added as far as I know. So it's safe for us to throw those quirk entries away.

Yeah, it doesn't make automated test tools easy for this case, sorry.

Hope this helps.

greg k-h

Here's the patch you requested, but I recommend against applying it since it's the sort of comment that can easily become wrong, leading to missed bugs in future kernels. Thanks again for your help.

Best,
Rob

@@ -869,6 +869,9 @@
 
 void pci_fixup_device(int pass, struct pci_dev *dev)
 {
+	/* Note: Many of the hooks in pci_fixups are declared __init
+           or __devinit, but this is ok because, currently, none of
+           the devices with quirk hooks are hot-pluggable. */
 	pci_do_fixups(dev, pass, pcibios_fixups);
 	pci_do_fixups(dev, pass, pci_fixups);
 }

Hm, good point. Can you think of a better place for this that would have helped you out?

thanks,

greg k-h

Take two. It might not have prevented me from reporting the potential bug, but I would've known you'd thought about it, it might help future developers, and it's unlikely to become dangerously wrong. Thanks.

Best,
Rob

@@ -750,6 +750,9 @@
 
 /*
  *  The main table of quirks.
+ *
+ *  Note: any hooks for hotpluggable devices in this table must _NOT_
+ *        be declared __init.
  */
 
 static struct pci_fixup pci_fixups[] __devinitdata = {

Thanks, I've applied this patch.

greg k-h

In article <[email protected]>, Robert T. Johnson <[email protected]> wrote:

| On Tue, 2003-09-30 at 14:25, Greg KH wrote:
| > Hm, good point.  Can you think of a better place for this that would
| > have helped you out?
| 
| Take two.  It might not have prevented me from reporting the potential
| bug, but I would've known you'd thought about it, it might help future
| developers, and it's unlikely to become dangerously wrong.  Thanks.
| 
| Best,
| Rob
| 
| --- drivers/pci/quirks.c.orig	Tue Sep 30 14:17:40 2003
| +++ drivers/pci/quirks.c	Tue Sep 30 14:39:48 2003
| @@ -750,6 +750,9 @@
|  
|  /*
|   *  The main table of quirks.
| + *
| + *  Note: any hooks for hotpluggable devices in this table must _NOT_
| + *        be declared __init.
|   */
|  
|  static struct pci_fixup pci_fixups[] __devinitdata = {

Good job, that. Clear, and anyone who doesn't immediately see why probably is at a skill level to just say 'oh' and do what it says.

bill davidsen <[email protected]>
  CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.