[-mm patch] drivers/pci/quirks.c: cleanup

[-mm patch] drivers/pci/quirks.c: cleanup

[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

Signed-off-by: Adrian Bunk <bunk@stusta.de>

---

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

--- linux-2.6.20-rc1-mm1/drivers/pci/quirks.c.old	2006-12-19 04:12:39.000000000 +0100
+++ linux-2.6.20-rc1-mm1/drivers/pci/quirks.c	2006-12-19 04:59:22.000000000 +0100
@@ -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
@@ -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 );
 
 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
--- linux-2.6.20-rc1-mm1/drivers/pci/pci.c.old	2006-12-19 04:15:52.000000000 +0100
+++ linux-2.6.20-rc1-mm1/drivers/pci/pci.c	2006-12-19 04:16:00.000000000 +0100
@@ -1210,7 +1210,3 @@ EXPORT_SYMBOL(pci_save_state);
 EXPORT_SYMBOL(pci_restore_state);
 EXPORT_SYMBOL(pci_enable_wake);
 
-/* Quirk info */
-
-EXPORT_SYMBOL(isa_dma_bridge_buggy);
-EXPORT_SYMBOL(pci_pci_problems);

Re: [-mm patch] drivers/pci/quirks.c: cleanup

[Matthew Wilcox]

On Tue, Dec 19, 2006 at 05:13:15AM +0100, Adrian Bunk wrote:
> +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);

Re: [-mm patch] drivers/pci/quirks.c: cleanup

[Adrian Bunk]

On Tue, Dec 19, 2006 at 01:52:49AM -0700, Matthew Wilcox wrote:
> On Tue, Dec 19, 2006 at 05:13:15AM +0100, Adrian Bunk wrote:
> > +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);

I also saw this, but it's consistent through the file.

But if it's agreed upon, I can also change this.

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [-mm patch] drivers/pci/quirks.c: cleanup

[Jean Delvare]

Hi Adrian,

On Tue, 19 Dec 2006 05:13:15 +0100, Adrian Bunk wrote:
> 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:

> 
> Signed-off-by: Adrian Bunk <bunk@stusta.de>
> 
> ---
> 
>  drivers/pci/pci.c    |    4 ----
>  drivers/pci/quirks.c |   42 +++++++++++++++++-------------------------
>  2 files changed, 17 insertions(+), 29 deletions(-)
> 
> --- linux-2.6.20-rc1-mm1/drivers/pci/quirks.c.old	2006-12-19 04:12:39.000000000 +0100
> +++ linux-2.6.20-rc1-mm1/drivers/pci/quirks.c	2006-12-19 04:59:22.000000000 +0100
> @@ -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
> @@ -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.

>  
>  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
> --- linux-2.6.20-rc1-mm1/drivers/pci/pci.c.old	2006-12-19 04:15:52.000000000 +0100
> +++ linux-2.6.20-rc1-mm1/drivers/pci/pci.c	2006-12-19 04:16:00.000000000 +0100
> @@ -1210,7 +1210,3 @@ EXPORT_SYMBOL(pci_save_state);
>  EXPORT_SYMBOL(pci_restore_state);
>  EXPORT_SYMBOL(pci_enable_wake);
>  
> -/* Quirk info */
> -
> -EXPORT_SYMBOL(isa_dma_bridge_buggy);
> -EXPORT_SYMBOL(pci_pci_problems);
> -

Thanks,
-- 
Jean Delvare

Re: [-mm patch] drivers/pci/quirks.c: cleanup

[Adrian Bunk]

On Fri, Jan 05, 2007 at 09:52:33AM +0100, Jean Delvare wrote:
> Hi Adrian,
> 
> On Tue, 19 Dec 2006 05:13:15 +0100, Adrian Bunk wrote:
> > 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:
>...
> > @@ -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.
>...

Thanks for this information.

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?


> Thanks,
> Jean Delvare


cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [-mm patch] drivers/pci/quirks.c: cleanup

[Jean Delvare]

Hi Adrian,

On Sat, 6 Jan 2007 00:29:13 +0100, Adrian Bunk wrote:
> 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?

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.

Thanks,
-- 
Jean Delvare

Re: [-mm patch] drivers/pci/quirks.c: cleanup

[Mark M. Hoffman]

Hi Jean, Adrian, et. al.:

* Jean Delvare <khali@linux-fr.org> [2007-01-07 12:30:13 +0100]:
> Hi Adrian,
> 
> On Sat, 6 Jan 2007 00:29:13 +0100, Adrian Bunk wrote:
> > 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?
> 
> 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.

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
mhoffman@lightlink.com

Re: [-mm patch] drivers/pci/quirks.c: cleanup

[Mark M. Hoffman]

Hi Jean, Adrian:

> On Tue, 19 Dec 2006 05:13:15 +0100, Adrian Bunk wrote:
> > @@ -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 );

* Jean Delvare <khali@linux-fr.org> [2007-01-05 09:52:33 +0100]:
> 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.

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.

Regards,

-- 
Mark M. Hoffman
mhoffman@lightlink.com

Re: [-mm patch] drivers/pci/quirks.c: cleanup

[Jean Delvare]

Hi Mark,

On Sun, 7 Jan 2007 10:44:41 -0500, Mark M. Hoffman wrote:
> Hi Jean, Adrian:
> 
> > On Tue, 19 Dec 2006 05:13:15 +0100, Adrian Bunk wrote:
> > > @@ -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 );
> 
> * Jean Delvare <khali@linux-fr.org> [2007-01-05 09:52:33 +0100]:
> > 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.
> 
> 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.

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

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

BTW, the Intel (Asus) SMBus unhiding quirk also depends on the linking
order if I'm not mistaking, and quite frankly I don't see a way to
"fix" it if we decide to no longer trust the linking order.

-- 
Jean Delvare

Re: [-mm patch] drivers/pci/quirks.c: cleanup

[Mark M. Hoffman]

Jean:

* Jean Delvare <khali@linux-fr.org> [2007-01-08 12:10:55 +0100]:
> Hi Mark,
> 
> On Sun, 7 Jan 2007 10:44:41 -0500, Mark M. Hoffman wrote:
> > Hi Jean, Adrian:
> > 
> > > On Tue, 19 Dec 2006 05:13:15 +0100, Adrian Bunk wrote:
> > > > @@ -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 );
> > 
> > * Jean Delvare <khali@linux-fr.org> [2007-01-05 09:52:33 +0100]:
> > > 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.
> > 
> > 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.

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

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

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

I'll resend it shortly...

Regards,

-- 
Mark M. Hoffman
mhoffman@lightlink.com

[PATCH 2.6.20-rc4] i2c/pci: fix sis96x smbus quirk once and for all

[Mark M. Hoffman]

The sis96x SMBus PCI device depends on two different quirks to run
in a specific order.  Apart from being fragile, this was found to
actually break on (at least) recent FC4, FC5, and FC6 kernels.  This
patch fixes the quirks so that they work without relying on the
compiler and/or linker to put them in any specific order.

http://lists.lm-sensors.org/pipermail/lm-sensors/2006-April/015962.html
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=189719

I tested this patch.  Please apply.

Signed-off-by: Mark M. Hoffman <mhoffman@lightlink.com>

---
 drivers/pci/quirks.c |   14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

--- linux-2.6.orig/drivers/pci/quirks.c
+++ linux-2.6/drivers/pci/quirks.c
@@ -1117,10 +1117,11 @@ DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_I
 static void quirk_sis_96x_smbus(struct pci_dev *dev)
 {
 	u8 val = 0;
-	printk(KERN_INFO "Enabling SiS 96x SMBus.\n");
-	pci_read_config_byte(dev, 0x77, &val);
-	pci_write_config_byte(dev, 0x77, val & ~0x10);
 	pci_read_config_byte(dev, 0x77, &val);
+	if (val & 0x10) {
+		printk(KERN_INFO "Enabling SiS 96x SMBus.\n");
+		pci_write_config_byte(dev, 0x77, val & ~0x10);
+	}
 }
 
 /*
@@ -1152,11 +1153,12 @@ static void quirk_sis_503(struct pci_dev
 	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.. The 96x quirks are after
-	 * the 503 quirk in the quirk table, so they'll automatically
-	 * run and enable things like the SMBus device
+	 * Ok, it now shows up as a 96x.. run the 96x quirk by
+	 * hand in case it has already been processed.
+	 * (depends on link order, which is apparently not guaranteed)
 	 */
 	dev->device = devid;
+	quirk_sis_96x_smbus(dev);
 }
 
 static void __init quirk_sis_96x_compatible(struct pci_dev *dev)
-- 
Mark M. Hoffman
mhoffman@lightlink.com

Re: [-mm patch] drivers/pci/quirks.c: cleanup

[Jean Delvare]

Hi Mark,

On Mon, 8 Jan 2007 22:02:26 -0500, Mark M. Hoffman wrote:
> Jean:
> 
> * Jean Delvare <khali@linux-fr.org> [2007-01-08 12:10:55 +0100]:
> > Hi Mark,
> > 
> > On Sun, 7 Jan 2007 10:44:41 -0500, Mark M. Hoffman wrote:
> > > 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.
> 
> 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. ;)

I seem to remember I suggested an improvement to make your fix better,
but the fix was originally yours. Not that it really matters, 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.
> > 
> > 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.

I can't say, sorry, I'm no compilation expert. It just sounded logical
to me that the compiler should keep the code in the same order it found
it in the sources, but it looks like this quirks code is very special.

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

Depends, as far as I remember, workarounds for mainline gcc are OK, but
workarounds for distro-specific gcc are not. But one may argue that
your patch isn't adding a workaround but cleaning up the code, then it
no longer matters.

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

-- 
Jean Delvare

Re: [-mm patch] drivers/pci/quirks.c: cleanup

[Mark M. Hoffman]

Hi Jean:

> On Mon, 8 Jan 2007 22:02:26 -0500, Mark M. Hoffman wrote:
> > 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.

* Jean Delvare <khali@linux-fr.org> [2007-01-09 14:17:21 +0100]:
> 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.

[1] That's right, the first quirk actually changes the PCI device ID of the
LPC.  Unless it actually *is* older hardware... in which case the quirk just
tickles a reserved bit that is ignored.  Thank you SiS, that's just beautiful.

Regards,

-- 
Mark M. Hoffman
mhoffman@lightlink.com

Re: [-mm patch] drivers/pci/quirks.c: cleanup

[Jean Delvare]

Hi Mark,

On Tue, 9 Jan 2007 22:58:20 -0500, Mark M. Hoffman wrote:
> * Jean Delvare <khali@linux-fr.org> [2007-01-09 14:17:21 +0100]:
> > 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.

Ah, OK, I think I get it now, thanks for the clarification. I thought
that the quirks were processed in file order for all devices at once,
while I now understand they are processed for each device in turn. In
which case, indeed, as long as the host bridge PCI device is listed
before the ISA bridge PCI device (and as you suggest this appears to be
guaranteed), the Intel SMBus quirk will work fine, regardless of the
linking order.

-- 
Jean Delvare

[-mm patch] remove quirk_sis_96x_compatible()

[Adrian Bunk]

On Sun, Jan 07, 2007 at 10:40:49AM -0500, Mark M. Hoffman wrote:
> Hi Jean, Adrian, et. al.:
> 
> * Jean Delvare <khali@linux-fr.org> [2007-01-07 12:30:13 +0100]:
> > Hi Adrian,
> > 
> > On Sat, 6 Jan 2007 00:29:13 +0100, Adrian Bunk wrote:
>...
> > > 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.
> 
> 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.

> Regards,
> Mark M. Hoffman

cu
Adrian


<--  snip  -->


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 <bunk@stusta.de>

---

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

--- linux-2.6.20-rc4-mm1/drivers/pci/quirks.c.old	2007-01-14 09:58:01.000000000 +0100
+++ linux-2.6.20-rc4-mm1/drivers/pci/quirks.c	2007-01-14 09:58:37.000000000 +0100
@@ -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

Re: [-mm patch] remove quirk_sis_96x_compatible()

[Mark M. Hoffman]

Hi Adrian:

> On Sun, Jan 07, 2007 at 10:40:49AM -0500, Mark M. Hoffman wrote:
> > 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.

* Adrian Bunk <bunk@stusta.de> [2007-01-14 14:46:32 +0100]:
> 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 <bunk@stusta.de>

Acked-by: Mark M. Hoffman <mhoffman@lightlink.com>

> ---
> 
>  drivers/pci/quirks.c |   15 ---------------
>  1 file changed, 15 deletions(-)
> 
> --- linux-2.6.20-rc4-mm1/drivers/pci/quirks.c.old	2007-01-14 09:58:01.000000000 +0100
> +++ linux-2.6.20-rc4-mm1/drivers/pci/quirks.c	2007-01-14 09:58:37.000000000 +0100
> @@ -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

-- 
Mark M. Hoffman
mhoffman@lightlink.com