2.6.0-test6: a few __init bugs

2.6.0-test6: a few __init bugs

[Robert T. Johnson]

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.

Re: 2.6.0-test6: a few __init bugs

[Pavel Machek]

Hi!

> 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.

> ** 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.

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?]

Re: 2.6.0-test6: a few __init bugs

[Greg KH]

On Mon, Sep 29, 2003 at 02:58:12PM -0700, Robert T. Johnson wrote:
> ** 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.

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

Re: 2.6.0-test6: a few __init bugs

[Robert T. Johnson]

On Tue, 2003-09-30 at 12:11, Greg KH wrote:
> Hm, care to send in a patch that adds a comment to that __init call so
> that others don't make the same mistake in a year or so?

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

--- drivers/pci/quirks.c.orig	Tue Sep 30 14:17:40 2003
+++ drivers/pci/quirks.c	Tue Sep 30 14:16:57 2003
@@ -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);
 }

Re: 2.6.0-test6: a few __init bugs

[Greg KH]

On Tue, Sep 30, 2003 at 02:20:53PM -0700, Robert T. Johnson wrote:
> On Tue, 2003-09-30 at 12:11, Greg KH wrote:
> > Hm, care to send in a patch that adds a comment to that __init call so
> > that others don't make the same mistake in a year or so?
> 
> 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.

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

thanks,

greg k-h

Re: 2.6.0-test6: a few __init bugs

[Robert T. Johnson]

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 = {

Re: 2.6.0-test6: a few __init bugs

[Greg KH]

On Tue, Sep 30, 2003 at 02:42:09PM -0700, Robert T. Johnson 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.

Thanks, I've applied this patch.

greg k-h

Re: 2.6.0-test6: a few __init bugs

[bill davidsen]

In article <1064958129.5264.237.camel@dooby.cs.berkeley.edu>,
Robert T. Johnson <rtjohnso@eecs.berkeley.edu> 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 <davidsen@tmr.com>
  CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.