Patch: 2.5.45 PCI Fixups for PCI HotPlug

Patch: 2.5.45 PCI Fixups for PCI HotPlug

[Lee, Jung-Ik]

[-- Attachment #1: Type: text/plain, Size: 337 bytes --]

Hi Greg,

The following patch changes function scopes only but fixes kernel dump on
Hot-Add of PCI bridge cards.

Thanks,
J.I. Lee
Enterprise Server Tech.
Intel Corp.

 arch/i386/pci/fixup.c |    4 ++--
 drivers/pci/quirks.c  |   50
+++++++++++++++++++++++++-------------------------
 2 files changed, 27 insertions(+), 27 deletions(-)


[-- Attachment #2: php_fix.diff --]
[-- Type: application/octet-stream, Size: 7991 bytes --]

diff -urN linux-2.5.45-ia64-021031.org/arch/i386/pci/fixup.c linux-2.5.45-ia64-021031-phpa/arch/i386/pci/fixup.c
--- linux-2.5.45-ia64-021031.org/arch/i386/pci/fixup.c	Wed Oct 30 16:41:56 2002
+++ linux-2.5.45-ia64-021031-phpa/arch/i386/pci/fixup.c	Sat Nov  2 16:22:15 2002
@@ -138,7 +138,7 @@
 #define VIA_8363_KL133_REVISION_ID 0x81
 #define VIA_8363_KM133_REVISION_ID 0x84
 
-static void __init pci_fixup_via_northbridge_bug(struct pci_dev *d)
+static void __devinit pci_fixup_via_northbridge_bug(struct pci_dev *d)
 {
 	u8 v;
 	u8 revision;
@@ -180,7 +180,7 @@
  * system to PCI bus no matter what are their window settings, so they are
  * "transparent" (or subtractive decoding) from programmers point of view.
  */
-static void __init pci_fixup_transparent_bridge(struct pci_dev *dev)
+static void __devinit pci_fixup_transparent_bridge(struct pci_dev *dev)
 {
 	if ((dev->class >> 8) == PCI_CLASS_BRIDGE_PCI &&
 	    (dev->device & 0xff00) == 0x2400)
diff -urN linux-2.5.45-ia64-021031.org/drivers/pci/quirks.c linux-2.5.45-ia64-021031-phpa/drivers/pci/quirks.c
--- linux-2.5.45-ia64-021031.org/drivers/pci/quirks.c	Wed Oct 30 16:41:36 2002
+++ linux-2.5.45-ia64-021031-phpa/drivers/pci/quirks.c	Sat Nov  2 16:27:32 2002
@@ -23,7 +23,7 @@
 
 /* Deal with broken BIOS'es that neglect to enable passive release,
    which can cause problems in combination with the 82441FX/PPro MTRRs */
-static void __init quirk_passive_release(struct pci_dev *dev)
+static void __devinit quirk_passive_release(struct pci_dev *dev)
 {
 	struct pci_dev *d = NULL;
 	unsigned char dlc;
@@ -50,7 +50,7 @@
 
 int isa_dma_bridge_buggy;		/* Exported */
     
-static void __init quirk_isa_dma_hangs(struct pci_dev *dev)
+static void __devinit quirk_isa_dma_hangs(struct pci_dev *dev)
 {
 	if (!isa_dma_bridge_buggy) {
 		isa_dma_bridge_buggy=1;
@@ -64,7 +64,7 @@
  *	Chipsets where PCI->PCI transfers vanish or hang
  */
 
-static void __init quirk_nopcipci(struct pci_dev *dev)
+static void __devinit quirk_nopcipci(struct pci_dev *dev)
 {
 	if((pci_pci_problems&PCIPCI_FAIL)==0)
 	{
@@ -77,7 +77,7 @@
  *	Triton requires workarounds to be used by the drivers
  */
  
-static void __init quirk_triton(struct pci_dev *dev)
+static void __devinit quirk_triton(struct pci_dev *dev)
 {
 	if((pci_pci_problems&PCIPCI_TRITON)==0)
 	{
@@ -96,7 +96,7 @@
  *	Updated based on further information from the site and also on
  *	information provided by VIA 
  */
-static void __init quirk_vialatency(struct pci_dev *dev)
+static void __devinit quirk_vialatency(struct pci_dev *dev)
 {
 	struct pci_dev *p;
 	u8 rev;
@@ -150,7 +150,7 @@
  *	VIA Apollo VP3 needs ETBF on BT848/878
  */
  
-static void __init quirk_viaetbf(struct pci_dev *dev)
+static void __devinit quirk_viaetbf(struct pci_dev *dev)
 {
 	if((pci_pci_problems&PCIPCI_VIAETBF)==0)
 	{
@@ -158,7 +158,7 @@
 		pci_pci_problems|=PCIPCI_VIAETBF;
 	}
 }
-static void __init quirk_vsfx(struct pci_dev *dev)
+static void __devinit quirk_vsfx(struct pci_dev *dev)
 {
 	if((pci_pci_problems&PCIPCI_VSFX)==0)
 	{
@@ -173,7 +173,7 @@
  *	at least
  */
  
-static void __init quirk_natoma(struct pci_dev *dev)
+static void __devinit quirk_natoma(struct pci_dev *dev)
 {
 	if((pci_pci_problems&PCIPCI_NATOMA)==0)
 	{
@@ -187,7 +187,7 @@
  *  If it's needed, re-allocate the region.
  */
 
-static void __init quirk_s3_64M(struct pci_dev *dev)
+static void __devinit quirk_s3_64M(struct pci_dev *dev)
 {
 	struct resource *r = &dev->resource[0];
 
@@ -197,7 +197,7 @@
 	}
 }
 
-static void __init quirk_io_region(struct pci_dev *dev, unsigned region, unsigned size, int nr)
+static void __devinit quirk_io_region(struct pci_dev *dev, unsigned region, unsigned size, int nr)
 {
 	region &= ~(size-1);
 	if (region) {
@@ -222,7 +222,7 @@
  *	0xE0 (64 bytes of ACPI registers)
  *	0xE2 (32 bytes of SMB registers)
  */
-static void __init quirk_ali7101_acpi(struct pci_dev *dev)
+static void __devinit quirk_ali7101_acpi(struct pci_dev *dev)
 {
 	u16 region;
 
@@ -237,7 +237,7 @@
  *	0x40 (64 bytes of ACPI registers)
  *	0x90 (32 bytes of SMB registers)
  */
-static void __init quirk_piix4_acpi(struct pci_dev *dev)
+static void __devinit quirk_piix4_acpi(struct pci_dev *dev)
 {
 	u32 region;
 
@@ -251,7 +251,7 @@
  * VIA ACPI: One IO region pointed to by longword at
  *	0x48 or 0x20 (256 bytes of ACPI registers)
  */
-static void __init quirk_vt82c586_acpi(struct pci_dev *dev)
+static void __devinit quirk_vt82c586_acpi(struct pci_dev *dev)
 {
 	u8 rev;
 	u32 region;
@@ -270,7 +270,7 @@
  *	0x70 (128 bytes of hardware monitoring register)
  *	0x90 (16 bytes of SMB registers)
  */
-static void __init quirk_vt82c686_acpi(struct pci_dev *dev)
+static void __devinit quirk_vt82c686_acpi(struct pci_dev *dev)
 {
 	u16 hm;
 	u32 smb;
@@ -297,7 +297,7 @@
  * TODO: When we have device-specific interrupt routers,
  * this code will go away from quirks.
  */
-static void __init quirk_via_ioapic(struct pci_dev *dev)
+static void __devinit quirk_via_ioapic(struct pci_dev *dev)
 {
 	u8 tmp;
 	
@@ -338,7 +338,7 @@
  * value of the ACPI SCI interrupt is only done for convenience.
  *	-jgarzik
  */
-static void __init quirk_via_acpi(struct pci_dev *d)
+static void __devinit quirk_via_acpi(struct pci_dev *d)
 {
 	/*
 	 * VIA ACPI device: SCI IRQ line in PCI config byte 0x42
@@ -350,7 +350,7 @@
 		d->irq = irq;
 }
 
-static void __init quirk_via_irqpic(struct pci_dev *dev)
+static void __devinit quirk_via_irqpic(struct pci_dev *dev)
 {
 	u8 irq, new_irq = dev->irq & 0xf;
 
@@ -377,7 +377,7 @@
  *
  * We mask out all r/wc bits, too.
  */
-static void __init quirk_piix3_usb(struct pci_dev *dev)
+static void __devinit quirk_piix3_usb(struct pci_dev *dev)
 {
 	u16 legsup;
 
@@ -392,7 +392,7 @@
  * We need to switch it off to be able to recognize the real
  * type of the chip.
  */
-static void __init quirk_vt82c598_id(struct pci_dev *dev)
+static void __devinit quirk_vt82c598_id(struct pci_dev *dev)
 {
 	pci_write_config_byte(dev, 0xfc, 0);
 	pci_read_config_word(dev, PCI_DEVICE_ID, &dev->device);
@@ -404,7 +404,7 @@
  * do this even if the Linux CardBus driver is not loaded, because
  * the Linux i82365 driver does not (and should not) handle CardBus.
  */
-static void __init quirk_cardbus_legacy(struct pci_dev *dev)
+static void __devinit quirk_cardbus_legacy(struct pci_dev *dev)
 {
 	if ((PCI_CLASS_BRIDGE_CARDBUS << 8) ^ dev->class)
 		return;
@@ -421,7 +421,7 @@
  * of course. However the advice is demonstrably good even if so..
  */
  
-static void __init quirk_amd_ioapic(struct pci_dev *dev)
+static void __devinit quirk_amd_ioapic(struct pci_dev *dev)
 {
 	u8 rev;
 
@@ -441,7 +441,7 @@
  * who turn it off!
  */
  
-static void __init quirk_amd_ordering(struct pci_dev *dev)
+static void __devinit quirk_amd_ordering(struct pci_dev *dev)
 {
 	u32 pcic;
 	pci_read_config_dword(dev, 0x4C, &pcic);
@@ -464,14 +464,14 @@
  *	nothing gets put too close to it.
  */
 
-static void __init quirk_dunord ( struct pci_dev * dev )
+static void __devinit quirk_dunord ( struct pci_dev * dev )
 {
 	struct resource * r = & dev -> resource [ 1 ];
 	r -> start = 0;
 	r -> end = 0xffffff;
 }
 
-static void __init quirk_transparent_bridge(struct pci_dev *dev)
+static void __devinit quirk_transparent_bridge(struct pci_dev *dev)
 {
 	dev->transparent = 1;
 }
@@ -480,7 +480,7 @@
  *  The main table of quirks.
  */
 
-static struct pci_fixup pci_fixups[] __initdata = {
+static struct pci_fixup pci_fixups[] __devinitdata = {
 	{ PCI_FIXUP_HEADER,	PCI_VENDOR_ID_DUNORD,	PCI_DEVICE_ID_DUNORD_I3000,	quirk_dunord },
 	{ PCI_FIXUP_FINAL,	PCI_VENDOR_ID_INTEL,	PCI_DEVICE_ID_INTEL_82441,	quirk_passive_release },
 	{ PCI_FIXUP_FINAL,	PCI_VENDOR_ID_INTEL,	PCI_DEVICE_ID_INTEL_82441,	quirk_passive_release },

Re: Patch: 2.5.45 PCI Fixups for PCI HotPlug

[Greg KH]

On Sat, Nov 02, 2002 at 04:56:58PM -0800, Lee, Jung-Ik wrote:
> Hi Greg,
> 
> The following patch changes function scopes only but fixes kernel dump on
> Hot-Add of PCI bridge cards.

Applied, thanks.

Hm, in looking at this, I know the majority of people who want
CONFIG_HOTPLUG probably do not run with CONFIG_PCI_HOTPLUG as the
hardware's still quite rare.  To force those people to keep around all
of the PCI quirks functions and tables after init happens, is a bit
cruel.  I wonder if it's time to start having different subsystems
modify __devinit depending on their config variables.

Does this sound like a good idea?  If so, I can probably knock up
something for the PCI code pretty easily (yes, I'll keep in mind CardBus
stuff, not all hotplug pci is on servers...)

__pci_devinit anyone?  :)

thanks,

greg k-h

Re: Patch: 2.5.45 PCI Fixups for PCI HotPlug

[Adam J. Richter]

[-- Attachment #1: Type: text/plain, Size: 1699 bytes --]

Greg KH wrote:
>Hm, in looking at this, I know the majority of people who want
>CONFIG_HOTPLUG probably do not run with CONFIG_PCI_HOTPLUG as the
>hardware's still quite rare.  To force those people to keep around all
>of the PCI quirks functions and tables after init happens, is a bit
>cruel.  I wonder if it's time to start having different subsystems
>modify __devinit depending on their config variables.

	Are there PCI bridge cards that use all of those?  For
example, I thought that Triton was a series of Intel motherboard
chipsets for 586 processors.  Perhaps you only need to keep a
few of those routines.

	Jung-Ik: perhaps you could to an lspci and an "lspci -n" on
your machine when the bridge card is plugged in, which should provide
enough information to determine which routines you really need to
keep.

>Does this sound like a good idea?  If so, I can probably knock up
>something for the PCI code pretty easily (yes, I'll keep in mind CardBus
>stuff, not all hotplug pci is on servers...)
>
>__pci_devinit anyone?  :)

	I posted a patch to define __usbinit and cousins to the
linux-kernel mailing list about two and a half years ago.  The change
has been sitting in my <linux/init.h> since then.  I just did a string
replacement to make the matching pci calls.  Here an an untested patch
that shows both.

	I believe that, theoretically, all __dev{init,exit}{,func}
should only be __dev<bus>{init,exit}{,func}.

-- 
Adam J. Richter     __     ______________   575 Oroville Road
adam@yggdrasil.com     \ /                  Milpitas, California 95035
+1 408 309-6081         | g g d r a s i l   United States of America
                         "Free Software For The Rest Of Us."

[-- Attachment #2: diffs --]
[-- Type: text/plain, Size: 1063 bytes --]

--- linux-2.5.45/include/linux/init.h	2002-10-30 16:41:39.000000000 -0800
+++ linux/include/linux/init.h	2002-11-03 03:59:35.000000000 -0800
@@ -196,4 +196,33 @@
 #define __devexit_p(x) NULL
 #endif
 
+#ifdef CONFIG_PCI_HOTPLUG
+#define __pcidevinit
+#define __pcidevinitdata
+#define __pcidevexit
+#define __pcidevexitdata
+#else
+#define __pcidevinit		__init
+#define __pcidevinitdata	__initdata
+#define __pcidevexit		__exit
+#define __pcidevexitdata	__exitdata
+#endif
+
+/* Warning: without CONFIG_USB_HOTPLUG, you will also not support
+   hot plugging of USB *controllers*, even though they are usually PCI
+   devices, because they will generate USB hotplug events for their
+   attached devices when they (the controllers) are inserted or removed.
+*/
+#ifdef CONFIG_USB_HOTPLUG
+#define __usbdevinit
+#define __usbdevinitdata
+#define __usbdevexit
+#define __usbdevexitdata
+#else
+#define __usbdevinit		__init
+#define __usbdevinitdata	__initdata
+#define __usbdevexit		__exit
+#define __usbdevexitdata	__exitdata
+#endif
+
 #endif /* _LINUX_INIT_H */

RE: Patch: 2.5.45 PCI Fixups for PCI HotPlug

[Lee, Jung-Ik]

> Greg KH wrote:
> >Hm, in looking at this, I know the majority of people who want
> >CONFIG_HOTPLUG probably do not run with CONFIG_PCI_HOTPLUG as the
> >hardware's still quite rare.  To force those people to keep 
> around all
> >of the PCI quirks functions and tables after init happens, is a bit
> >cruel.  I wonder if it's time to start having different subsystems
> >modify __devinit depending on their config variables.
> 
> 	Are there PCI bridge cards that use all of those?  For
> example, I thought that Triton was a series of Intel motherboard
> chipsets for 586 processors.  Perhaps you only need to keep a
> few of those routines.
> 
> 	Jung-Ik: perhaps you could to an lspci and an "lspci -n" on
> your machine when the bridge card is plugged in, which should provide
> enough information to determine which routines you really need to
> keep.

That sounds a quick fix for now but Greg's __pci_devinit seems to be the
right solution.

thanks,
J.I.

RE: Patch: 2.5.45 PCI Fixups for PCI HotPlug

[Adam J. Richter]

>> Greg KH wrote:
>> >Hm, in looking at this, I know the majority of people who want
>> >CONFIG_HOTPLUG probably do not run with CONFIG_PCI_HOTPLUG as the
>> >hardware's still quite rare.  To force those people to keep 
>> around all
>> >of the PCI quirks functions and tables after init happens, is a bit
>> >cruel.  I wonder if it's time to start having different subsystems
>> >modify __devinit depending on their config variables.
>> 
>> 	Are there PCI bridge cards that use all of those?  For
>> example, I thought that Triton was a series of Intel motherboard
>> chipsets for 586 processors.  Perhaps you only need to keep a
>> few of those routines.
>> 
>> 	Jung-Ik: perhaps you could to an lspci and an "lspci -n" on
>> your machine when the bridge card is plugged in, which should provide
>> enough information to determine which routines you really need to
>> keep.

>That sounds a quick fix for now but Greg's __pci_devinit seems to be the
>right solution.

	There is no reason to use __pci_devinit for chipsets that are
only soldered into motherboards.  I believe there are only a few of
those quirk handlers that CONFIG_PCI_HOTPLUG users really need to
retain in their kernels.

	I would appreciate it if you would do the lspci commands I requested
or just send the contents of your /proc/bus/pci/devices file.

Adam J. Richter     __     ______________   575 Oroville Road
adam@yggdrasil.com     \ /                  Milpitas, California 95035
+1 408 309-6081         | g g d r a s i l   United States of America
                         "Free Software For The Rest Of Us."

RE: Patch: 2.5.45 PCI Fixups for PCI HotPlug

[Alan Cox]

On Mon, 2002-11-04 at 18:23, Adam J. Richter wrote:
> 	There is no reason to use __pci_devinit for chipsets that are
> only soldered into motherboards.  I believe there are only a few of
> those quirk handlers that CONFIG_PCI_HOTPLUG users really need to
> retain in their kernels.

You might be suprised what "motherboard" chips turn up in docking
stations.My TP600 for example has an IBM southbridge in it.

RE: Patch: 2.5.45 PCI Fixups for PCI HotPlug

[Adam J. Richter]

Alan Cox writes:

>On Mon, 2002-11-04 at 18:23, Adam J. Richter wrote:
>> 	There is no reason to use __pci_devinit for chipsets that are
>> only soldered into motherboards.  I believe there are only a few of
>> those quirk handlers that CONFIG_PCI_HOTPLUG users really need to
>> retain in their kernels.

>You might be suprised what "motherboard" chips turn up in docking
>stations.My TP600 for example has an IBM southbridge in it.

	What are you advocating?  Do you want all quirk routines
marked __devinit?  Wouldn't you agree at least that if we can
establish that device *cannot* be hot plugged, that its quirk routine
can be __init?

	I see at least three levels of "hardware reality support":

		1. Configurations known to be in use.
		2. Possible configurations of hardware products known to exist
		3. Hardware products not known to exist, based on chips
		   and specs known to exist in other real hardware products.

	Jeff Garzik recently said that he doesn't want to incorporate
hot plugging for arbitrary standard PCI cards until he hears about a
hotplug configuration that uses that card (I'm cc'ing Jeff so he can
correct me if I misunderstand). That would put the kernel at level 1
in the above list.

	You seem to be advocating the third level, but maybe I'm
reading too much into your note.

	For what it's worth, although I think that individual
trade-offs can lead to some drivers being exceptions in one direction
or another, I would prefer level 2 for odd kernel versions so that we
can get reports about things like your TP600's chip set.  I do think
that patches for hotplug support for arbitrary regular PCI card should
be accepted now that there are backplanes that can hot plug them.

	By the way, does your TP600 docking station use one of the
mechanisms that is included by CONFIG_PCI_HOTPLUG or is it compiled in
either way (like CardBus)?

	If:
	- Your docking station were not controlled by CONFIG_PCI_HOTPLUG
	- and your kernel were not configured CONFIG_PCI_HOTPLUG
	- and your docking station needed a quirk routine
	- and the quirk routine were marked with the proposed __pcidevinit 
	- and you were to plug in your station after boot

	...then you would get a kernel segment fault, even if you have
CONFIG_HOTPLUG defined.

Adam J. Richter     __     ______________   575 Oroville Road
adam@yggdrasil.com     \ /                  Milpitas, California 95035
+1 408 309-6081         | g g d r a s i l   United States of America
                         "Free Software For The Rest Of Us."

Re: Patch: 2.5.45 PCI Fixups for PCI HotPlug

[Jeff Garzik]

Adam J. Richter wrote:

>	Jeff Garzik recently said that he doesn't want to incorporate
>hot plugging for arbitrary standard PCI cards until he hears about a
>hotplug configuration that uses that card (I'm cc'ing Jeff so he can
>correct me if I misunderstand). That would put the kernel at level 1
>in the above list.
>

No, I was talking more specifically about de2104x driver.

For general drivers I prefer __devinit out of general laziness:  if 
there is not an active and kernel-clueful maintainer, then it's better 
to mark everything __devinit [with the wastage it implies].  But for a 
few drivers with obviously provable/disprovable cases, __init may be better.

So it IMO matters WRT maintainer and testability issues as well as 
strictly kernel/technical issues.

    Jeff

Re: Patch: 2.5.45 PCI Fixups for PCI HotPlug

[Ivan Kokshaysky]

On Mon, Nov 04, 2002 at 12:29:37PM -0800, Adam J. Richter wrote:
> 	What are you advocating?  Do you want all quirk routines
> marked __devinit?  Wouldn't you agree at least that if we can
> establish that device *cannot* be hot plugged, that its quirk routine
> can be __init?

You cannot mark individual quirk routines differently as long as they
belong in the same quirk list. If the list is __devinitdata and some
of routines in it are __init, you'll have an oops in the hotplug path.

What we need is an additional quirk list, say, "hotplug_pci_fixups"
and a global flag "init_gone" (probably free_initmem() should set it).
Then we'll have

void pci_fixup_device(int pass, struct pci_dev *dev)
{
-	pci_do_fixups(dev, pass, pcibios_fixups);
-	pci_do_fixups(dev, pass, pci_fixups);
+	if (!init_gone) {
+		pci_do_fixups(dev, pass, pcibios_fixups);
+		pci_do_fixups(dev, pass, pci_fixups);
+	}
+	pci_do_fixups(dev, pass, hotplug_pci_fixups);
}

Perhaps arch specific "hotplug_pcibios_fixups" is also needed, as archs
may work around the same pci bug differently.

What should go into the new list is another story. Obviously the fixups for
host bridges must be __init, as well as some south bridges that look like
pci devices but actually aren't.

Ivan.

Re: Patch: 2.5.45 PCI Fixups for PCI HotPlug

[Adam J. Richter]

Ivan Kokshaysky wrote:
>On Mon, Nov 04, 2002 at 12:29:37PM -0800, Adam J. Richter wrote:
>You cannot mark individual quirk routines differently as long as they
>belong in the same quirk list. If the list is __devinitdata and some
>of routines in it are __init, you'll have an oops in the hotplug path.

	If pci_do_fixups determines that f->vendor and f->device do
not match the device in question, it will not call the corresponding
f->hook, so it is OK for that f->hook to point to the address of a
discarded __init routine that now contains garbage as long as the
ID's will not match any hot plugged device.

Adam J. Richter     __     ______________   575 Oroville Road
adam@yggdrasil.com     \ /                  Milpitas, California 95035
+1 408 309-6081         | g g d r a s i l   United States of America
                         "Free Software For The Rest Of Us."

Re: Patch: 2.5.45 PCI Fixups for PCI HotPlug

[Ivan Kokshaysky]

On Tue, Nov 05, 2002 at 05:17:10AM -0800, Adam J. Richter wrote:
> 	If pci_do_fixups determines that f->vendor and f->device do
> not match the device in question, it will not call the corresponding
> f->hook, so it is OK for that f->hook to point to the address of a
> discarded __init routine that now contains garbage as long as the
> ID's will not match any hot plugged device.

Unless f->device == PCI_ANY_ID. Plus you won't be able to discard
the list itself.

Ivan.

Re: Patch: 2.5.45 PCI Fixups for PCI HotPlug

[Adam J. Richter]

Ivan Kokshaysky wrote:
>On Tue, Nov 05, 2002 at 05:17:10AM -0800, Adam J. Richter wrote:
>> 	If pci_do_fixups determines that f->vendor and f->device do
>> not match the device in question, it will not call the corresponding
>> f->hook, so it is OK for that f->hook to point to the address of a
>> discarded __init routine that now contains garbage as long as the
>> ID's will not match any hot plugged device.
                 ^^^^^

>Unless f->device == PCI_ANY_ID.

	I said "match", not "equal."


>Plus you won't be able to discard the list itself.

	Your original claim was:

| You cannot mark individual quirk routines differently as long as they
| belong in the same quirk list. If the list is __devinitdata and some
| of routines in it are __init, you'll have an oops in the hotplug path.

	I think that that has been disproven and you are now arguing a
different claim: that while the list *can* safely be __devinitdata
with some __init routines, it might save some space to split the lists
so the list that hold the __init routines can also be __initdata.  I'd
just like to be clear so that we understand that your proposal no
longer has the urgency of being needed to solve Jung-Ik's crash.  That
is just a matter of changing one or two routines to __devinit and
pci{,bios}_fixups[] to __devinitdata (or to __pcidevinit{,data} if you
want to add that facility).

	Given that we're now examining the trade-offs of an optional
change, let's try to quantify it, starting with the benefits.

	sizeof(struct pci_fixup) == 12 or 16 bytes

	pci_fixups[] has 42 entries == 504 or 672 bytes

	i386 pcibios_fixups[] has 17 entries == 204 bytes

	If half of these entries would remain __init, so that would
save 354 bytes on i386.  Probably more than half can remain __init.
So, on x86, we would expect that ~250-350 bytes of table space could
remain in __initdata.  You will also gain a negligible speed
improvement in PCI hotplug insertion (not really a critical path, but
I mention it for completeness).

	Now let's look at the costs.  The system_running global
variable will already tell you if __init{,data} are no longer valid,
so there is no need to add a new variable, your proposed routine
would become:

static struct pci_fixup pci_fixups_initial[] __initdata = {
	...
};

static struct pci_fixup pci_fixups[] __pcidevinitdata = {
	...
};

void __pcidevinit pci_fixup_device(int pass, struct pci_dev *dev)
{
        pci_do_fixups(dev, pass, pcibios_fixups);
	if (!system_running)
		pci_do_fixups(dev, pass, pci_fixups_initial);
        pci_do_fixups(dev, pass, pci_fixups);
}

	(Change __pcidevinit{,data} to __devinit{data,} if that facility
is not added.)

	Your additional code will probably be ~30 bytes.  We do not
have to count the space for the terminating entries for the additional
tables, because those would be __init.  There may be some implicit
ordering beyond PCI_FIXUP_{HEADER,FINAL} for a routine or two that you
will need to duplicate between the __init and __devinit tables or
accomodate in some other way, so let's assume 20 bytes there.

	Subtracting the negatives from the positives, your proposal
would probably save ~300 bytes on x86, or ~200 bytes if you only split
pci_fixups[] (as above), more as additional __init fixups appear or if
struct pci_fixup grows.

	There is also the question of whether you want to have a third
set of tables for __pcidevinitdata routines.  Probably, if you did the
carbus_legacy call separately, you would only need __initdata and
__pcidevinitdata tables, so CONFIG_PCI_HOTPLUG users would keep
one table and regular CONFIG_PCI users would keep none, saving
them another 200-300 bytes (i.e., getting back to the current
utilization of no tables after ).

	I like the idea of your change.  For what it's worth,
I recommend the following.

	1. Immediately change pci{,bios}_fixups[] to __devinitdata
(or __pcidevinitdata if that facility is added, as it appears
that CardBus insertion calls pci_insert_device directly, skipping
pci_fixup_device).  This is a real bug.  pci_fixup_device is a
__devinit routine walking an __initdata table.

	2. If Jung-Ik gets around to posting his lspci or
/proc/bus/pci/devices, change his particular fixup routines to
__devinit.

	3. If you want to pursue splitting pci_fixups into
__initdata and __pcidevinitdata (or __devinitdata) as an
optimization, fine.  I'd recommend that you start with just
splitting pci_fixups[] so you do not have to coordinate with
a bunch of architectures initially.  It's simple and I think
200 bytes for CONFIG_PCI_HOTPLUG users is worth one "if" and
splitting one table.  It would not surprise me if Linus or,
better, whoever you go through to feed patches to Linus (I
guess Greg is volunteering for that) might want to wait
until after the "freeze", although I have no objection to
splitting pci_fixups[] now.

Adam J. Richter     __     ______________   575 Oroville Road
adam@yggdrasil.com     \ /                  Milpitas, California 95035
+1 408 309-6081         | g g d r a s i l   United States of America
                         "Free Software For The Rest Of Us."