drivers/pci/quirks.c: cleanup

drivers/pci/quirks.c static void __devinit quirk_nopcipci(str · line 100

Quoted 2 lines · Adrian Bunk

drivers/pci/quirks.c renamed from drivers/pci/quirks.c.old +2 -0

+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_5597,		quirk_nopcipci );
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_496,		quirk_nopcipci );

Why all the crazy spacing?

+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_SI, PCI_DEVICE_ID_SI_5597, quirk_nopcipci);

This patch contains the following cleanups:
- move all EXPORT_SYMBOL's directly below the code they are exporting
- move all DECLARE_PCI_FIXUP_*'s directly below the functions they are calling

Signed-off-by Adrian Bunk <[email protected]>

---

 drivers/pci/pci.c    |    4 ----
 drivers/pci/quirks.c |   42 +++++++++++++++++-------------------------
 2 files changed, 17 insertions(+), 29 deletions(-)

drivers/pci/quirks.c renamed from drivers/pci/quirks.c.old +13 -4

@@ -61,7 +61,8 @@ DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_I
 
     This appears to be BIOS not version dependent. So presumably there is a 
     chipset level fix */
-int isa_dma_bridge_buggy;		/* Exported */
+int isa_dma_bridge_buggy;
+EXPORT_SYMBOL(isa_dma_bridge_buggy);
 
 static void __devinit quirk_isa_dma_hangs(struct pci_dev *dev)
 {

@@ -83,6 +84,7 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_NE
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_NEC,	PCI_DEVICE_ID_NEC_CBUS_3,	quirk_isa_dma_hangs );
 
 int pci_pci_problems;
+EXPORT_SYMBOL(pci_pci_problems);
 
 /*
  *	Chipsets where PCI->PCI transfers vanish or hang

@@ -94,6 +96,8 @@ static void __devinit quirk_nopcipci(str
 		pci_pci_problems |= PCIPCI_FAIL;
 	}
 }
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_5597,		quirk_nopcipci );
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_496,		quirk_nopcipci );
 
 static void __devinit quirk_nopciamd(struct pci_dev *dev)
 {

@@ -105,9 +109,6 @@ static void __devinit quirk_nopciamd(str
 		pci_pci_problems |= PCIAGP_FAIL;
 	}
 }
-
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_5597,		quirk_nopcipci );
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_496,		quirk_nopcipci );
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AMD,	PCI_DEVICE_ID_AMD_8151_0,	quirk_nopciamd );
 
 /*

@@ -1122,6 +1123,14 @@ static void quirk_sis_96x_smbus(struct p
 	pci_write_config_byte(dev, 0x77, val & ~0x10);
 	pci_read_config_byte(dev, 0x77, &val);
 }
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_961,		quirk_sis_96x_smbus );
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_962,		quirk_sis_96x_smbus );
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_963,		quirk_sis_96x_smbus );
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_LPC,		quirk_sis_96x_smbus );
+DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_961,		quirk_sis_96x_smbus );
+DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_962,		quirk_sis_96x_smbus );
+DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_963,		quirk_sis_96x_smbus );
+DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_LPC,		quirk_sis_96x_smbus );
 
 /*
  * ... This is further complicated by the fact that some SiS96x south

drivers/pci/quirks.c renamed from drivers/pci/quirks.c.old +2 -21

 static void __init quirk_sis_96x_compatible(struct pci_dev *dev)
 {

@@ -1170,8 +1181,6 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_S
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_651,		quirk_sis_96x_compatible );
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_735,		quirk_sis_96x_compatible );
 
-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_503,		quirk_sis_503 );
-DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_503,		quirk_sis_503 );
 /*
  * On ASUS A8V and A8V Deluxe boards, the onboard AC97 audio controller
  * and MC97 modem controller are disabled when a second PCI soundcard is

@@ -1202,21 +1211,8 @@ static void asus_hides_ac97_lpc(struct p
 	}
 }
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_VIA,	PCI_DEVICE_ID_VIA_8237, asus_hides_ac97_lpc );
-
-
-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_961,		quirk_sis_96x_smbus );
-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_962,		quirk_sis_96x_smbus );
-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_963,		quirk_sis_96x_smbus );
-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_LPC,		quirk_sis_96x_smbus );
-
 DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_VIA,	PCI_DEVICE_ID_VIA_8237, asus_hides_ac97_lpc );
 
-
-DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_961,		quirk_sis_96x_smbus );
-DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_962,		quirk_sis_96x_smbus );
-DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_963,		quirk_sis_96x_smbus );
-DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_LPC,		quirk_sis_96x_smbus );
-
 #if defined(CONFIG_ATA) || defined(CONFIG_ATA_MODULE)
 
 /*

@@ -1261,7 +1257,6 @@ static void quirk_jmicron_dualfn(struct 
 			break;
 	}
 }
-
 DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_JMICRON, PCI_ANY_ID, quirk_jmicron_dualfn);
 DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_JMICRON, PCI_ANY_ID, quirk_jmicron_dualfn);
 

@@ -1405,6 +1400,7 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_IN
 
 
 int pcie_mch_quirk;
+EXPORT_SYMBOL(pcie_mch_quirk);
 
 static void __devinit quirk_pcie_mch(struct pci_dev *pdev)
 {

@@ -1649,6 +1645,7 @@ void pci_fixup_device(enum pci_fixup_pas
 	}
 	pci_do_fixups(dev, start, end);
 }
+EXPORT_SYMBOL(pci_fixup_device);
 
 /* Enable 1k I/O space granularity on the Intel P64H2 */
 static void __devinit quirk_p64h2_1k_io(struct pci_dev *dev)

@@ -1791,8 +1788,3 @@ static void __devinit quirk_nvidia_ck804
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_NVIDIA, PCI_DEVICE_ID_NVIDIA_CK804_PCIE,
 			quirk_nvidia_ck804_msi_ht_cap);
 #endif /* CONFIG_PCI_MSI */
-
-EXPORT_SYMBOL(pcie_mch_quirk);
-#ifdef CONFIG_HOTPLUG
-EXPORT_SYMBOL(pci_fixup_device);
-#endif

Hi Adrian,

Quoted 4 lines · Adrian Bunk

This patch contains the following cleanups:
- move all EXPORT_SYMBOL's directly below the code they are exporting
- move all DECLARE_PCI_FIXUP_*'s directly below the functions they are calling

Thanks for doing this cleanup, it was really needed. I think you didn't get everything right though:
...

Quoted 16 lines

drivers/pci/quirks.c renamed from drivers/pci/quirks.c.old +8 -0

@@ -1122,6 +1123,14 @@ static void quirk_sis_96x_smbus(struct p
 	pci_write_config_byte(dev, 0x77, val & ~0x10);
 	pci_read_config_byte(dev, 0x77, &val);
 }
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_961,		quirk_sis_96x_smbus );
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_962,		quirk_sis_96x_smbus );
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_963,		quirk_sis_96x_smbus );
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_LPC,		quirk_sis_96x_smbus );
+DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_961,		quirk_sis_96x_smbus );
+DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_962,		quirk_sis_96x_smbus );
+DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_963,		quirk_sis_96x_smbus );
+DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_LPC,		quirk_sis_96x_smbus );
 
 /*
  * ... This is further complicated by the fact that some SiS96x south

drivers/pci/quirks.c static void quirk_sis_503(struct pci_dev · line 1171

Quoted 5 lines

drivers/pci/quirks.c renamed from drivers/pci/quirks.c.old +2 -0

@@ -1158,6 +1167,8 @@ static void quirk_sis_503(struct pci_dev
 	 */
 	dev->device = devid;
 }
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_503,		quirk_sis_503 );
+DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_503,		quirk_sis_503 );

Was this patch tested on the SiS-based boards which need these quirks? I think you broke them. If I remember correctly, quirk_sis_503() must be called before quirk_sis_96x_smbus() for some boards to work properly, and we currently rely on the linking order to guarantee that. Likewise, quirk_sis_96x_compatible() should be called before quirk_sis_503() otherwise the warning message in quirk_sis_503() will no longer be correct.

So if you want to put the calls right after the quirk functions, you need to reorder the functions themselves as well. Feel free to add a comment explaining the order requirement so that nobody breaks it accidentally again in the future.
...

Thanks,
Jean Delvare

While looking at the code, I also noted the following:

quirk_sis_96x_compatible() is pretty useless since all it does is to set a static variable that is only used in a printk().

quirk_sis_96x_compatible() was added with:

2003/05/13 13:48:50-07:00 mhoffman
[PATCH] i2c: Add SiS96x I2C/SMBus driver

This patch adds support for the SMBus of SiS96x south bridges. It is based on i2c-sis645.c from the lm sensors project, which never made it into an official kernel and was anyway mis-named.

This driver works on my SiS 645/961 board vs w83781d.

It's usage in

static void __init quirk_sis_503_smbus(struct pci_dev *dev)

{
       if (sis_96x_compatible)
               quirk_sis_96x_smbus(dev);
}

Was removed in

Author: torvalds <torvalds>
Date:   Thu Oct 30 19:03:38 2003 +0000

Stop SIS 96x chips from lying about themselves.

Some machines with the SIS 96x southbridge have it set up to claim it is a SIS 503 chip. That breaks irq routing logic among other things. Fix it properly by making everybody aware of the duplicity.

Was this intentional (and quirk_sis_96x_compatible() should be removed), or is this a bug that should be fixed?

Hi Adrian,

Quoted 37 lines · Adrian Bunk

While looking at the code, I also noted the following:

quirk_sis_96x_compatible() is pretty useless since all it does is to set a static variable that is only used in a printk().

quirk_sis_96x_compatible() was added with:

2003/05/13 13:48:50-07:00 mhoffman
[PATCH] i2c: Add SiS96x I2C/SMBus driver

This patch adds support for the SMBus of SiS96x south bridges. It is based on i2c-sis645.c from the lm sensors project, which never made it into an official kernel and was anyway mis-named.

This driver works on my SiS 645/961 board vs w83781d.

It's usage in

static void __init quirk_sis_503_smbus(struct pci_dev *dev)

{
       if (sis_96x_compatible)
               quirk_sis_96x_smbus(dev);
}

Was removed in

Author: torvalds <torvalds>
Date:   Thu Oct 30 19:03:38 2003 +0000

Stop SIS 96x chips from lying about themselves.

Quoted 8 lines

Some machines with the SIS 96x southbridge have it set up to claim it is a SIS 503 chip. That breaks irq routing logic among other things. Fix it properly by making everybody aware of the duplicity.

Was this intentional (and quirk_sis_96x_compatible() should be removed), or is this a bug that should be fixed?

I noticed this too in April 2006, see:
http://lists.lm-sensors.org/pipermail/lm-sensors/2006-April/016016.html

Quoting myself back then:
"The whole sis_96x_compatible stuff looks superfluous now. It was used before 2.6.0-test10, but we could certainly get rid of it now."

I do not think there is a bug here, or someone would have complained by now. Note though that I do not have a SiS-based motherboard to test on. Mark may be able to help with testing.

Hi Jean, Adrian, et. al.:

Quoted 3 lines · Jean Delvare

Hi Adrian,

On Sat, 6 Jan 2007 00:29:13 +0100, Adrian Bunk wrote:

...

Quoted 10 lines

Quoted 2 lines

Was this intentional (and quirk_sis_96x_compatible() should be removed), or is this a bug that should be fixed?

I noticed this too in April 2006, see:
http://lists.lm-sensors.org/pipermail/lm-sensors/2006-April/016016.html

Quoting myself back then:
"The whole sis_96x_compatible stuff looks superfluous now. It was used before 2.6.0-test10, but we could certainly get rid of it now."

Quoted 3 lines

I do not think there is a bug here, or someone would have complained by now. Note though that I do not have a SiS-based motherboard to test on. Mark may be able to help with testing.

It's just cruft from the original quirk. The "compatible" printk could have had value as a diagnostic in case the new quirk didn't work for some reason, but I never saw any complaints about it (apart from the link order problem, which is something different.) It's safe to remove by now.

Regards,
Mark M. Hoffman

Quoted 4 lines

It's just cruft from the original quirk. The "compatible" printk could have had value as a diagnostic in case the new quirk didn't work for some reason, but I never saw any complaints about it (apart from the link order problem, which is something different.) It's safe to remove by now.

Below is a patch to remove it.

Since 2.6.0-test10, all quirk_sis_96x_compatible() had any effect on was a printk().

This patch therefore removes it.

Signed-off-by Adrian Bunk <[email protected]>

---

 drivers/pci/quirks.c |   15 ---------------
 1 file changed, 15 deletions(-)

drivers/pci/quirks.c renamed from drivers/pci/quirks.c.old +0 -15

@@ -1141,8 +1141,6 @@
  *
  * We can also enable the sis96x bit in the discovery register..
  */
-static int __devinitdata sis_96x_compatible = 0;
-
 #define SIS_DETECT_REGISTER 0x40
 
 static void quirk_sis_503(struct pci_dev *dev)

@@ -1158,9 +1156,6 @@
 		return;
 	}
 
-	/* Make people aware that we changed the config.. */
-	printk(KERN_WARNING "Uncovering SIS%x that hid as a SIS503 (compatible=%d)\n", devid, sis_96x_compatible);
-
 	/*
 	 * Ok, it now shows up as a 96x.. run the 96x quirk by
 	 * hand in case it has already been processed.

@@ -1172,16 +1167,6 @@
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_503,		quirk_sis_503 );
 DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_503,		quirk_sis_503 );
 
-static void __init quirk_sis_96x_compatible(struct pci_dev *dev)
-{
-	sis_96x_compatible = 1;
-}
-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_645,		quirk_sis_96x_compatible );
-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_646,		quirk_sis_96x_compatible );
-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_648,		quirk_sis_96x_compatible );
-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_650,		quirk_sis_96x_compatible );
-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_651,		quirk_sis_96x_compatible );
-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_735,		quirk_sis_96x_compatible );
 
 /*
  * On ASUS A8V and A8V Deluxe boards, the onboard AC97 audio controller

Quoted 21 lines

drivers/pci/quirks.c renamed from drivers/pci/quirks.c.old +10 -0

@@ -1122,6 +1123,14 @@ static void quirk_sis_96x_smbus(struct p
 	pci_write_config_byte(dev, 0x77, val & ~0x10);
 	pci_read_config_byte(dev, 0x77, &val);
 }
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_961,		quirk_sis_96x_smbus );
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_962,		quirk_sis_96x_smbus );
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_963,		quirk_sis_96x_smbus );
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_LPC,		quirk_sis_96x_smbus );
+DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_961,		quirk_sis_96x_smbus );
+DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_962,		quirk_sis_96x_smbus );
+DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_963,		quirk_sis_96x_smbus );
+DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_LPC,		quirk_sis_96x_smbus );
 
 /*
  * ... This is further complicated by the fact that some SiS96x south

@@ -1158,6 +1167,8 @@ static void quirk_sis_503(struct pci_dev
 	 */
 	dev->device = devid;
 }
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_503,		quirk_sis_503 );
+DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_503,		quirk_sis_503 );

Was this patch tested on the SiS-based boards which need these quirks? I think you broke them. If I remember correctly, quirk_sis_503() must be called before quirk_sis_96x_smbus() for some boards to work properly, and we currently rely on the linking order to guarantee that. Likewise, quirk_sis_96x_compatible() should be called before quirk_sis_503() otherwise the warning message in quirk_sis_503() will no longer be correct.

So if you want to put the calls right after the quirk functions, you need to reorder the functions themselves as well. Feel free to add a comment explaining the order requirement so that nobody breaks it accidentally again in the future.

Hi Jean, Adrian:

Quoted 22 lines · Adrian Bunk

Quoted 21 lines

drivers/pci/quirks.c renamed from drivers/pci/quirks.c.old +10 -0

@@ -1122,6 +1123,14 @@ static void quirk_sis_96x_smbus(struct p
 	pci_write_config_byte(dev, 0x77, val & ~0x10);
 	pci_read_config_byte(dev, 0x77, &val);
 }
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_961,		quirk_sis_96x_smbus );
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_962,		quirk_sis_96x_smbus );
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_963,		quirk_sis_96x_smbus );
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_LPC,		quirk_sis_96x_smbus );
+DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_961,		quirk_sis_96x_smbus );
+DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_962,		quirk_sis_96x_smbus );
+DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_963,		quirk_sis_96x_smbus );
+DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_LPC,		quirk_sis_96x_smbus );
 
 /*
  * ... This is further complicated by the fact that some SiS96x south

@@ -1158,6 +1167,8 @@ static void quirk_sis_503(struct pci_dev
 	 */
 	dev->device = devid;
 }
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_503,		quirk_sis_503 );
+DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_503,		quirk_sis_503 );

Quoted 8 lines · Jean Delvare

Was this patch tested on the SiS-based boards which need these quirks? I think you broke them. If I remember correctly, quirk_sis_503() must be called before quirk_sis_96x_smbus() for some boards to work properly, and we currently rely on the linking order to guarantee that. Likewise, quirk_sis_96x_compatible() should be called before quirk_sis_503() otherwise the warning message in quirk_sis_503() will no longer be correct.

Quoted 4 lines

So if you want to put the calls right after the quirk functions, you need to reorder the functions themselves as well. Feel free to add a comment explaining the order requirement so that nobody breaks it accidentally again in the future.

It is fragile for this code to depend on link order; Adrian's obvious and trivial cleanups broke it. Not only that, but some FC kernels had/have the link order reversed such that this quirk is broken anyway.

I sent a patch for this back in May:
http://lists.lm-sensors.org/pipermail/lm-sensors/2006-May/016113.html

There was some discussion on the linux-pci mailing list as well; can't seem to find an archive of that though. Basically, it was not understood how the FC kernels could have a reversed link order. I never followed up on it, my bad.

At any rate, can we please get the patch above applied? I will send a new one if necessary.

Hi Mark,

Quoted 39 lines · Mark M. Hoffman

Hi Jean, Adrian:

Quoted 22 lines · Adrian Bunk

Quoted 21 lines

drivers/pci/quirks.c renamed from drivers/pci/quirks.c.old +10 -0

@@ -1122,6 +1123,14 @@ static void quirk_sis_96x_smbus(struct p
 	pci_write_config_byte(dev, 0x77, val & ~0x10);
 	pci_read_config_byte(dev, 0x77, &val);
 }
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_961,		quirk_sis_96x_smbus );
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_962,		quirk_sis_96x_smbus );
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_963,		quirk_sis_96x_smbus );
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_LPC,		quirk_sis_96x_smbus );
+DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_961,		quirk_sis_96x_smbus );
+DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_962,		quirk_sis_96x_smbus );
+DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_963,		quirk_sis_96x_smbus );
+DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_LPC,		quirk_sis_96x_smbus );
 
 /*
  * ... This is further complicated by the fact that some SiS96x south

@@ -1158,6 +1167,8 @@ static void quirk_sis_503(struct pci_dev
 	 */
 	dev->device = devid;
 }
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_503,		quirk_sis_503 );
+DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_SI,	PCI_DEVICE_ID_SI_503,		quirk_sis_503 );

Quoted 12 lines · Jean Delvare

Was this patch tested on the SiS-based boards which need these quirks? I think you broke them. If I remember correctly, quirk_sis_503() must be called before quirk_sis_96x_smbus() for some boards to work properly, and we currently rely on the linking order to guarantee that. Likewise, quirk_sis_96x_compatible() should be called before quirk_sis_503() otherwise the warning message in quirk_sis_503() will no longer be correct.

So if you want to put the calls right after the quirk functions, you need to reorder the functions themselves as well. Feel free to add a comment explaining the order requirement so that nobody breaks it accidentally again in the future.

Quoted 3 lines

It is fragile for this code to depend on link order; Adrian's obvious and trivial cleanups broke it. Not only that, but some FC kernels had/have the link order reversed such that this quirk is broken anyway.

The former problem would be addressed just fine by a proper ordering (as Adrian's patch was attempting to bring) and a comment explaining the dependency.

Quoted 6 lines

I sent a patch for this back in May:
http://lists.lm-sensors.org/pipermail/lm-sensors/2006-May/016113.html

There was some discussion on the linux-pci mailing list as well; can't seem to find an archive of that though. Basically, it was not understood how the FC kernels could have a reversed link order. I never followed up on it, my bad.

As long as it isn't explained, I call it a compiler bug in FC.

Quoted 2 lines

At any rate, can we please get the patch above applied? I will send a new one if necessary.

This is a PCI patch, so I'm not the one picking it. I seem to remember Greg was fine with the patch except for the comment about the linking order.

Jean:

Quoted 7 lines · Jean Delvare

Hi Mark,

Quoted 3 lines · Mark M. Hoffman

It is fragile for this code to depend on link order; Adrian's obvious and trivial cleanups broke it. Not only that, but some FC kernels had/have the link order reversed such that this quirk is broken anyway.

Quoted 3 lines

The former problem would be addressed just fine by a proper ordering (as Adrian's patch was attempting to bring) and a comment explaining the dependency.

That's still fragile. Someone is bound to reorder the stupid things by mistake (again). I'm tired of screwing around with this quirk already. The patch that I sent last May would have fixed it permanently. And the funny part is that *you* suggested the fix. ;)

Quoted 8 lines

Quoted 6 lines

I sent a patch for this back in May:
http://lists.lm-sensors.org/pipermail/lm-sensors/2006-May/016113.html

There was some discussion on the linux-pci mailing list as well; can't seem to find an archive of that though. Basically, it was not understood how the FC kernels could have a reversed link order. I never followed up on it, my bad.

As long as it isn't explained, I call it a compiler bug in FC.

1) What standard specifies the link order of objects in a module? I have seen other compilers reorder objects this way.

2) So what if it was a compiler bug? I guess we don't allow patches to work around compiler bugs.

3) I've just confirmed that this quirk is still broken on recent FC6 kernels. Perhaps they should have picked my patch out of their bugzilla, but they didn't.

Quoted 2 lines

3) I've just confirmed that this quirk is still broken on recent FC6 kernels. Perhaps they should have picked my patch out of their bugzilla, but they didn't.

I am worried about the Intel/Asus SMBus quirk then, which affects many more users than the SiS SMBus one, and would suffer from a reordering as well.

Quoted 3 lines · Jean Delvare

I am worried about the Intel/Asus SMBus quirk then, which affects many more users than the SiS SMBus one, and would suffer from a reordering as well.

Intel/Asus users on FC[456] would surely have screamed if that was true. But I was curious so I looked deeper. There is a fundamental difference between the Intel SMBus quirks and the SiS SMBus quirk...

Intel:
1) The first quirk keys off the host bridge, setting a flag.
2) The second quirk keys off the LPC, enabling the SMBus if the flag was set.

SiS:
1) The first quirk keys off the *old* LPC ID... this causes the ID to change.[1]
2) The second quirk keys off the *new* LPC ID; this one enables the SMBus.

In the SiS case, both quirks key off the *same* *device*, but with potentially different IDs. The quirk list ordering matters there because the list is scanned only once per device.

For the Intel case, the only ordering that matters is that the host bridge device is added [pci_device_add()] before the LPC; AFAICT, that is reliable, perhaps even by definition.

So I don't think you have to worry about the Intel SMBus quirks.