[PATCH] 6700/6702PXH quirk

[PATCH] 6700/6702PXH quirk

[Kristen Accardi]

On the 6700/6702 PXH part, a MSI may get corrupted if an ACPI hotplug
driver and SHPC driver in MSI mode are used together.  This patch will
prevent MSI from being enabled for the SHPC.  

I made this patch more generic than just shpc because I thought it was
possible that other devices in the system might need to add themselves
to the msi black list.

Signed-off-by: Kristen Carlson Accardi <kristen.c.accardi@intel.com>

diff -uprN -X linux-2.6.13-rc4/Documentation/dontdiff linux-2.6.13-rc4/drivers/pci/msi.c linux-2.6.13-rc4-pxhquirk/drivers/pci/msi.c
--- linux-2.6.13-rc4/drivers/pci/msi.c	2005-07-28 15:44:44.000000000 -0700
+++ linux-2.6.13-rc4-pxhquirk/drivers/pci/msi.c	2005-08-04 12:09:44.000000000 -0700
@@ -38,6 +38,32 @@ int vector_irq[NR_VECTORS] = { [0 ... NR
 u8 irq_vector[NR_IRQ_VECTORS] = { FIRST_DEVICE_VECTOR , 0 };
 #endif
 
+
+LIST_HEAD(msi_quirk_list);
+
+struct msi_quirk 
+{
+	struct list_head list;
+	struct pci_dev *dev;
+};
+
+
+int msi_add_quirk(struct pci_dev *dev)
+{
+	struct msi_quirk *quirk;
+
+	quirk = (struct msi_quirk *) kmalloc(sizeof(*quirk), GFP_KERNEL);
+	if (!quirk)
+		return -ENOMEM;
+	
+	INIT_LIST_HEAD(&quirk->list);
+	quirk->dev = dev;
+	list_add(&quirk->list, &msi_quirk_list);
+	return 0;
+}
+
+
+
 static void msi_cache_ctor(void *p, kmem_cache_t *cache, unsigned long flags)
 {
 	memset(p, 0, NR_IRQS * sizeof(struct msi_desc));
@@ -453,7 +479,7 @@ static void enable_msi_mode(struct pci_d
 	}
 }
 
-static void disable_msi_mode(struct pci_dev *dev, int pos, int type)
+void disable_msi_mode(struct pci_dev *dev, int pos, int type)
 {
 	u16 control;
 
@@ -695,10 +721,16 @@ int pci_enable_msi(struct pci_dev* dev)
 {
 	int pos, temp, status = -EINVAL;
 	u16 control;
+	struct msi_quirk *quirk;
 
 	if (!pci_msi_enable || !dev)
  		return status;
 
+	list_for_each_entry(quirk, &msi_quirk_list, list) {
+		if (quirk->dev == dev)
+			return -EINVAL;
+	}
+
 	temp = dev->irq;
 
 	if ((status = msi_init()) < 0)
@@ -1127,3 +1159,5 @@ EXPORT_SYMBOL(pci_enable_msi);
 EXPORT_SYMBOL(pci_disable_msi);
 EXPORT_SYMBOL(pci_enable_msix);
 EXPORT_SYMBOL(pci_disable_msix);
+EXPORT_SYMBOL(disable_msi_mode);
+EXPORT_SYMBOL(msi_add_quirk);
diff -uprN -X linux-2.6.13-rc4/Documentation/dontdiff linux-2.6.13-rc4/drivers/pci/quirks.c linux-2.6.13-rc4-pxhquirk/drivers/pci/quirks.c
--- linux-2.6.13-rc4/drivers/pci/quirks.c	2005-07-28 15:44:44.000000000 -0700
+++ linux-2.6.13-rc4-pxhquirk/drivers/pci/quirks.c	2005-08-04 12:09:55.000000000 -0700
@@ -21,6 +21,10 @@
 #include <linux/acpi.h>
 #include "pci.h"
 
+
+extern void disable_msi_mode(struct pci_dev *dev, int pos, int type);
+extern int msi_add_quirk(struct pci_dev *dev);
+
 /* Deal with broken BIOS'es that neglect to enable passive release,
    which can cause problems in combination with the 82441FX/PPro MTRRs */
 static void __devinit quirk_passive_release(struct pci_dev *dev)
@@ -1267,6 +1271,30 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_IN
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,	PCI_DEVICE_ID_INTEL_E7320_MCH,	quirk_pcie_mch );
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,	PCI_DEVICE_ID_INTEL_E7525_MCH,	quirk_pcie_mch );
 
+
+/* 
+ * It's possible for the MSI to get corrupted if shpc and acpi
+ * are used together on certain PXH-based systems.
+ */
+static void __devinit quirk_pcie_pxh(struct pci_dev *dev)
+{
+	disable_msi_mode(dev, pci_find_capability(dev, PCI_CAP_ID_MSI),
+					PCI_CAP_ID_MSI);
+	if (!msi_add_quirk(dev)) 
+		printk(KERN_WARNING "PCI: PXH quirk detected, disabling MSI for SHPC device\n");
+	else {
+		pci_msi_quirk = 1;
+		printk(KERN_WARNING "PCI: PXH quirk detected, unable to disable MSI for SHPC device, disabling MSI for all devices\n");
+	}
+			
+}
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL,	PCI_DEVICE_ID_INTEL_PXHD_0,	quirk_pcie_pxh);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL,	PCI_DEVICE_ID_INTEL_PXHD_1,	quirk_pcie_pxh);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL,	PCI_DEVICE_ID_INTEL_PXH_0,	quirk_pcie_pxh);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL,	PCI_DEVICE_ID_INTEL_PXH_1,	quirk_pcie_pxh);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL,	PCI_DEVICE_ID_INTEL_PXHV,	quirk_pcie_pxh);
+
+
 static void __devinit quirk_netmos(struct pci_dev *dev)
 {
 	unsigned int num_parallel = (dev->subsystem_device & 0xf0) >> 4;
diff -uprN -X linux-2.6.13-rc4/Documentation/dontdiff linux-2.6.13-rc4/include/linux/pci_ids.h linux-2.6.13-rc4-pxhquirk/include/linux/pci_ids.h
--- linux-2.6.13-rc4/include/linux/pci_ids.h	2005-07-28 15:44:44.000000000 -0700
+++ linux-2.6.13-rc4-pxhquirk/include/linux/pci_ids.h	2005-08-02 13:58:53.000000000 -0700
@@ -2281,6 +2281,11 @@
 #define PCI_VENDOR_ID_INTEL		0x8086
 #define PCI_DEVICE_ID_INTEL_EESSC	0x0008
 #define PCI_DEVICE_ID_INTEL_21145	0x0039
+#define PCI_DEVICE_ID_INTEL_PXHD_0	0x0320
+#define PCI_DEVICE_ID_INTEL_PXHD_1	0x0321
+#define PCI_DEVICE_ID_INTEL_PXH_0	0x0329
+#define PCI_DEVICE_ID_INTEL_PXH_1	0x032A
+#define PCI_DEVICE_ID_INTEL_PXHV	0x032C
 #define PCI_DEVICE_ID_INTEL_82375	0x0482
 #define PCI_DEVICE_ID_INTEL_82424	0x0483
 #define PCI_DEVICE_ID_INTEL_82378	0x0484

Re: [PATCH] 6700/6702PXH quirk

[Bjorn Helgaas]

On Friday 05 August 2005 10:27 am, Kristen Accardi wrote:
> On the 6700/6702 PXH part, a MSI may get corrupted if an ACPI hotplug
> driver and SHPC driver in MSI mode are used together.  This patch will
> prevent MSI from being enabled for the SHPC.  

Can you outline the scenario that causes the corruption?  This patch
feels like a band-aid over a deeper problem.

Re: [PATCH] 6700/6702PXH quirk

[Kristen Accardi]

On Fri, 2005-08-05 at 11:12 -0600, Bjorn Helgaas wrote:
> On Friday 05 August 2005 10:27 am, Kristen Accardi wrote:
> > On the 6700/6702 PXH part, a MSI may get corrupted if an ACPI hotplug
> > driver and SHPC driver in MSI mode are used together.  This patch will
> > prevent MSI from being enabled for the SHPC.  
> 
> Can you outline the scenario that causes the corruption?  This patch
> feels like a band-aid over a deeper problem.

This is a workaround to a hardware problem.  If a MSI interrupt occurs
while the ACPI driver is doing a config read of the hotplug capabilities
register, the MSI interrupt will be corrupted, and the MSI interrupt
will never make it to the MCH.  This causes the hotplug device to not be
recognized.

Re: [PATCH] 6700/6702PXH quirk

[Greg KH]

On Fri, Aug 05, 2005 at 09:27:42AM -0700, Kristen Accardi wrote:
> @@ -1127,3 +1159,5 @@ EXPORT_SYMBOL(pci_enable_msi);
>  EXPORT_SYMBOL(pci_disable_msi);
>  EXPORT_SYMBOL(pci_enable_msix);
>  EXPORT_SYMBOL(pci_disable_msix);
> +EXPORT_SYMBOL(disable_msi_mode);
> +EXPORT_SYMBOL(msi_add_quirk);

Why do these need to be exported?  It doesn't look like you are trying
to access these from a module, or do you have a patch that uses them
somewhere else?

thanks,

greg k-h

Re: [PATCH] 6700/6702PXH quirk

[Kristen Accardi]

On Fri, 2005-08-05 at 11:35 -0700, Greg KH wrote:
> On Fri, Aug 05, 2005 at 09:27:42AM -0700, Kristen Accardi wrote:
> > @@ -1127,3 +1159,5 @@ EXPORT_SYMBOL(pci_enable_msi);
> >  EXPORT_SYMBOL(pci_disable_msi);
> >  EXPORT_SYMBOL(pci_enable_msix);
> >  EXPORT_SYMBOL(pci_disable_msix);
> > +EXPORT_SYMBOL(disable_msi_mode);
> > +EXPORT_SYMBOL(msi_add_quirk);
> 
> Why do these need to be exported?  It doesn't look like you are trying
> to access these from a module, or do you have a patch that uses them
> somewhere else?
> 
> thanks,
> 
> greg k-h

Oh... I thought I had to in order to access it from quirks.c.  You are
right, I don't need this, and we can always export later if modules want
to add to the msi_quirks list.

Signed-off-by: Kristen Carlson Accardi <kristen.c.accardi@intel.com>

diff -uprN -X linux-2.6.13-rc4/Documentation/dontdiff linux-2.6.13-rc4/drivers/pci/msi.c linux-2.6.13-rc4-pxhquirk/drivers/pci/msi.c
--- linux-2.6.13-rc4/drivers/pci/msi.c	2005-07-28 15:44:44.000000000 -0700
+++ linux-2.6.13-rc4-pxhquirk/drivers/pci/msi.c	2005-08-05 11:38:00.000000000 -0700
@@ -38,6 +38,32 @@ int vector_irq[NR_VECTORS] = { [0 ... NR
 u8 irq_vector[NR_IRQ_VECTORS] = { FIRST_DEVICE_VECTOR , 0 };
 #endif
 
+
+LIST_HEAD(msi_quirk_list);
+
+struct msi_quirk 
+{
+	struct list_head list;
+	struct pci_dev *dev;
+};
+
+
+int msi_add_quirk(struct pci_dev *dev)
+{
+	struct msi_quirk *quirk;
+
+	quirk = (struct msi_quirk *) kmalloc(sizeof(*quirk), GFP_KERNEL);
+	if (!quirk)
+		return -ENOMEM;
+	
+	INIT_LIST_HEAD(&quirk->list);
+	quirk->dev = dev;
+	list_add(&quirk->list, &msi_quirk_list);
+	return 0;
+}
+
+
+
 static void msi_cache_ctor(void *p, kmem_cache_t *cache, unsigned long flags)
 {
 	memset(p, 0, NR_IRQS * sizeof(struct msi_desc));
@@ -453,7 +479,7 @@ static void enable_msi_mode(struct pci_d
 	}
 }
 
-static void disable_msi_mode(struct pci_dev *dev, int pos, int type)
+void disable_msi_mode(struct pci_dev *dev, int pos, int type)
 {
 	u16 control;
 
@@ -695,10 +721,16 @@ int pci_enable_msi(struct pci_dev* dev)
 {
 	int pos, temp, status = -EINVAL;
 	u16 control;
+	struct msi_quirk *quirk;
 
 	if (!pci_msi_enable || !dev)
  		return status;
 
+	list_for_each_entry(quirk, &msi_quirk_list, list) {
+		if (quirk->dev == dev)
+			return -EINVAL;
+	}
+
 	temp = dev->irq;
 
 	if ((status = msi_init()) < 0)
diff -uprN -X linux-2.6.13-rc4/Documentation/dontdiff linux-2.6.13-rc4/drivers/pci/quirks.c linux-2.6.13-rc4-pxhquirk/drivers/pci/quirks.c
--- linux-2.6.13-rc4/drivers/pci/quirks.c	2005-07-28 15:44:44.000000000 -0700
+++ linux-2.6.13-rc4-pxhquirk/drivers/pci/quirks.c	2005-08-05 11:54:15.000000000 -0700
@@ -21,6 +21,10 @@
 #include <linux/acpi.h>
 #include "pci.h"
 
+
+void disable_msi_mode(struct pci_dev *dev, int pos, int type);
+int msi_add_quirk(struct pci_dev *dev);
+
 /* Deal with broken BIOS'es that neglect to enable passive release,
    which can cause problems in combination with the 82441FX/PPro MTRRs */
 static void __devinit quirk_passive_release(struct pci_dev *dev)
@@ -1267,6 +1271,30 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_IN
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,	PCI_DEVICE_ID_INTEL_E7320_MCH,	quirk_pcie_mch );
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,	PCI_DEVICE_ID_INTEL_E7525_MCH,	quirk_pcie_mch );
 
+
+/* 
+ * It's possible for the MSI to get corrupted if shpc and acpi
+ * are used together on certain PXH-based systems.
+ */
+static void __devinit quirk_pcie_pxh(struct pci_dev *dev)
+{
+	disable_msi_mode(dev, pci_find_capability(dev, PCI_CAP_ID_MSI),
+					PCI_CAP_ID_MSI);
+	if (!msi_add_quirk(dev)) 
+		printk(KERN_WARNING "PCI: PXH quirk detected, disabling MSI for SHPC device\n");
+	else {
+		pci_msi_quirk = 1;
+		printk(KERN_WARNING "PCI: PXH quirk detected, unable to disable MSI for SHPC device, disabling MSI for all devices\n");
+	}
+			
+}
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL,	PCI_DEVICE_ID_INTEL_PXHD_0,	quirk_pcie_pxh);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL,	PCI_DEVICE_ID_INTEL_PXHD_1,	quirk_pcie_pxh);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL,	PCI_DEVICE_ID_INTEL_PXH_0,	quirk_pcie_pxh);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL,	PCI_DEVICE_ID_INTEL_PXH_1,	quirk_pcie_pxh);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL,	PCI_DEVICE_ID_INTEL_PXHV,	quirk_pcie_pxh);
+
+
 static void __devinit quirk_netmos(struct pci_dev *dev)
 {
 	unsigned int num_parallel = (dev->subsystem_device & 0xf0) >> 4;
diff -uprN -X linux-2.6.13-rc4/Documentation/dontdiff linux-2.6.13-rc4/include/linux/pci_ids.h linux-2.6.13-rc4-pxhquirk/include/linux/pci_ids.h
--- linux-2.6.13-rc4/include/linux/pci_ids.h	2005-07-28 15:44:44.000000000 -0700
+++ linux-2.6.13-rc4-pxhquirk/include/linux/pci_ids.h	2005-08-02 13:58:53.000000000 -0700
@@ -2281,6 +2281,11 @@
 #define PCI_VENDOR_ID_INTEL		0x8086
 #define PCI_DEVICE_ID_INTEL_EESSC	0x0008
 #define PCI_DEVICE_ID_INTEL_21145	0x0039
+#define PCI_DEVICE_ID_INTEL_PXHD_0	0x0320
+#define PCI_DEVICE_ID_INTEL_PXHD_1	0x0321
+#define PCI_DEVICE_ID_INTEL_PXH_0	0x0329
+#define PCI_DEVICE_ID_INTEL_PXH_1	0x032A
+#define PCI_DEVICE_ID_INTEL_PXHV	0x032C
 #define PCI_DEVICE_ID_INTEL_82375	0x0482
 #define PCI_DEVICE_ID_INTEL_82424	0x0483
 #define PCI_DEVICE_ID_INTEL_82378	0x0484

Re: [PATCH] 6700/6702PXH quirk

[Kristen Accardi]

On Fri, 2005-08-05 at 11:35 -0700, Greg KH wrote:
> On Fri, Aug 05, 2005 at 09:27:42AM -0700, Kristen Accardi wrote:
> > @@ -1127,3 +1159,5 @@ EXPORT_SYMBOL(pci_enable_msi);
> >  EXPORT_SYMBOL(pci_disable_msi);
> >  EXPORT_SYMBOL(pci_enable_msix);
> >  EXPORT_SYMBOL(pci_disable_msix);
> > +EXPORT_SYMBOL(disable_msi_mode);
> > +EXPORT_SYMBOL(msi_add_quirk);
> 
> Why do these need to be exported?  It doesn't look like you are trying
> to access these from a module, or do you have a patch that uses them
> somewhere else?
> 
> thanks,
> 
> greg k-h

resend with changelog info:
On the 6700/6702 PXH part, a MSI may get corrupted if an ACPI hotplug
driver and SHPC driver in MSI mode are used together.  This patch will
prevent MSI from being enabled for the SHPC.  

I made this patch more generic than just shpc because I thought it was
possible that other devices in the system might need to add themselves
to the msi black list.

Signed-off-by: Kristen Carlson Accardi <kristen.c.accardi@intel.com>

diff -uprN -X linux-2.6.13-rc4/Documentation/dontdiff linux-2.6.13-rc4/drivers/pci/msi.c linux-2.6.13-rc4-pxhquirk/drivers/pci/msi.c
--- linux-2.6.13-rc4/drivers/pci/msi.c	2005-07-28 15:44:44.000000000 -0700
+++ linux-2.6.13-rc4-pxhquirk/drivers/pci/msi.c	2005-08-05 11:38:00.000000000 -0700
@@ -38,6 +38,32 @@ int vector_irq[NR_VECTORS] = { [0 ... NR
 u8 irq_vector[NR_IRQ_VECTORS] = { FIRST_DEVICE_VECTOR , 0 };
 #endif
 
+
+LIST_HEAD(msi_quirk_list);
+
+struct msi_quirk 
+{
+	struct list_head list;
+	struct pci_dev *dev;
+};
+
+
+int msi_add_quirk(struct pci_dev *dev)
+{
+	struct msi_quirk *quirk;
+
+	quirk = (struct msi_quirk *) kmalloc(sizeof(*quirk), GFP_KERNEL);
+	if (!quirk)
+		return -ENOMEM;
+	
+	INIT_LIST_HEAD(&quirk->list);
+	quirk->dev = dev;
+	list_add(&quirk->list, &msi_quirk_list);
+	return 0;
+}
+
+
+
 static void msi_cache_ctor(void *p, kmem_cache_t *cache, unsigned long flags)
 {
 	memset(p, 0, NR_IRQS * sizeof(struct msi_desc));
@@ -453,7 +479,7 @@ static void enable_msi_mode(struct pci_d
 	}
 }
 
-static void disable_msi_mode(struct pci_dev *dev, int pos, int type)
+void disable_msi_mode(struct pci_dev *dev, int pos, int type)
 {
 	u16 control;
 
@@ -695,10 +721,16 @@ int pci_enable_msi(struct pci_dev* dev)
 {
 	int pos, temp, status = -EINVAL;
 	u16 control;
+	struct msi_quirk *quirk;
 
 	if (!pci_msi_enable || !dev)
  		return status;
 
+	list_for_each_entry(quirk, &msi_quirk_list, list) {
+		if (quirk->dev == dev)
+			return -EINVAL;
+	}
+
 	temp = dev->irq;
 
 	if ((status = msi_init()) < 0)
diff -uprN -X linux-2.6.13-rc4/Documentation/dontdiff linux-2.6.13-rc4/drivers/pci/quirks.c linux-2.6.13-rc4-pxhquirk/drivers/pci/quirks.c
--- linux-2.6.13-rc4/drivers/pci/quirks.c	2005-07-28 15:44:44.000000000 -0700
+++ linux-2.6.13-rc4-pxhquirk/drivers/pci/quirks.c	2005-08-05 11:54:15.000000000 -0700
@@ -21,6 +21,10 @@
 #include <linux/acpi.h>
 #include "pci.h"
 
+
+void disable_msi_mode(struct pci_dev *dev, int pos, int type);
+int msi_add_quirk(struct pci_dev *dev);
+
 /* Deal with broken BIOS'es that neglect to enable passive release,
    which can cause problems in combination with the 82441FX/PPro MTRRs */
 static void __devinit quirk_passive_release(struct pci_dev *dev)
@@ -1267,6 +1271,30 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_IN
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,	PCI_DEVICE_ID_INTEL_E7320_MCH,	quirk_pcie_mch );
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,	PCI_DEVICE_ID_INTEL_E7525_MCH,	quirk_pcie_mch );
 
+
+/* 
+ * It's possible for the MSI to get corrupted if shpc and acpi
+ * are used together on certain PXH-based systems.
+ */
+static void __devinit quirk_pcie_pxh(struct pci_dev *dev)
+{
+	disable_msi_mode(dev, pci_find_capability(dev, PCI_CAP_ID_MSI),
+					PCI_CAP_ID_MSI);
+	if (!msi_add_quirk(dev)) 
+		printk(KERN_WARNING "PCI: PXH quirk detected, disabling MSI for SHPC device\n");
+	else {
+		pci_msi_quirk = 1;
+		printk(KERN_WARNING "PCI: PXH quirk detected, unable to disable MSI for SHPC device, disabling MSI for all devices\n");
+	}
+			
+}
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL,	PCI_DEVICE_ID_INTEL_PXHD_0,	quirk_pcie_pxh);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL,	PCI_DEVICE_ID_INTEL_PXHD_1,	quirk_pcie_pxh);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL,	PCI_DEVICE_ID_INTEL_PXH_0,	quirk_pcie_pxh);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL,	PCI_DEVICE_ID_INTEL_PXH_1,	quirk_pcie_pxh);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL,	PCI_DEVICE_ID_INTEL_PXHV,	quirk_pcie_pxh);
+
+
 static void __devinit quirk_netmos(struct pci_dev *dev)
 {
 	unsigned int num_parallel = (dev->subsystem_device & 0xf0) >> 4;
diff -uprN -X linux-2.6.13-rc4/Documentation/dontdiff linux-2.6.13-rc4/include/linux/pci_ids.h linux-2.6.13-rc4-pxhquirk/include/linux/pci_ids.h
--- linux-2.6.13-rc4/include/linux/pci_ids.h	2005-07-28 15:44:44.000000000 -0700
+++ linux-2.6.13-rc4-pxhquirk/include/linux/pci_ids.h	2005-08-02 13:58:53.000000000 -0700
@@ -2281,6 +2281,11 @@
 #define PCI_VENDOR_ID_INTEL		0x8086
 #define PCI_DEVICE_ID_INTEL_EESSC	0x0008
 #define PCI_DEVICE_ID_INTEL_21145	0x0039
+#define PCI_DEVICE_ID_INTEL_PXHD_0	0x0320
+#define PCI_DEVICE_ID_INTEL_PXHD_1	0x0321
+#define PCI_DEVICE_ID_INTEL_PXH_0	0x0329
+#define PCI_DEVICE_ID_INTEL_PXH_1	0x032A
+#define PCI_DEVICE_ID_INTEL_PXHV	0x032C
 #define PCI_DEVICE_ID_INTEL_82375	0x0482
 #define PCI_DEVICE_ID_INTEL_82424	0x0483
 #define PCI_DEVICE_ID_INTEL_82378	0x0484

Re: [PATCH] 6700/6702PXH quirk

[Andrew Morton]

Kristen Accardi <kristen.c.accardi@intel.com> wrote:
>
> ...
> On the 6700/6702 PXH part, a MSI may get corrupted if an ACPI hotplug
> driver and SHPC driver in MSI mode are used together.  This patch will
> prevent MSI from being enabled for the SHPC.  
> 
> I made this patch more generic than just shpc because I thought it was
> possible that other devices in the system might need to add themselves
> to the msi black list.
> 
> diff -uprN -X linux-2.6.13-rc4/Documentation/dontdiff linux-2.6.13-rc4/drivers/pci/msi.c linux-2.6.13-rc4-pxhquirk/drivers/pci/msi.c
> --- linux-2.6.13-rc4/drivers/pci/msi.c	2005-07-28 15:44:44.000000000 -0700
> +++ linux-2.6.13-rc4-pxhquirk/drivers/pci/msi.c	2005-08-05 11:38:00.000000000 -0700
> @@ -38,6 +38,32 @@ int vector_irq[NR_VECTORS] = { [0 ... NR
>  u8 irq_vector[NR_IRQ_VECTORS] = { FIRST_DEVICE_VECTOR , 0 };
>  #endif
>  
> +
> +LIST_HEAD(msi_quirk_list);
> +

Can't this have static scope?

> +struct msi_quirk 
> +{
> +	struct list_head list;
> +	struct pci_dev *dev;
> +};

We normally do

	struct msi_quirk {

> +
> +int msi_add_quirk(struct pci_dev *dev)
> +{
> +	struct msi_quirk *quirk;
> +
> +	quirk = (struct msi_quirk *) kmalloc(sizeof(*quirk), GFP_KERNEL);

kmalloc() returns void*, hence no typecast is needed.  In fact it's
undesirable because the cast defeats all typechecking.

> +	if (!quirk)
> +		return -ENOMEM;
> +	
> +	INIT_LIST_HEAD(&quirk->list);
> +	quirk->dev = dev;
> +	list_add(&quirk->list, &msi_quirk_list);
> +	return 0;
> +}

Does the list not need any locking?

> --- linux-2.6.13-rc4/drivers/pci/quirks.c	2005-07-28 15:44:44.000000000 -0700
> +++ linux-2.6.13-rc4-pxhquirk/drivers/pci/quirks.c	2005-08-05 11:54:15.000000000 -0700
> @@ -21,6 +21,10 @@
>  #include <linux/acpi.h>
>  #include "pci.h"
>  
> +
> +void disable_msi_mode(struct pci_dev *dev, int pos, int type);
> +int msi_add_quirk(struct pci_dev *dev);
> +

Please put these declarations in a .h file which is visible to the
implementations and to all users.

> +static void __devinit quirk_pcie_pxh(struct pci_dev *dev)
> +{
> +	disable_msi_mode(dev, pci_find_capability(dev, PCI_CAP_ID_MSI),
> +					PCI_CAP_ID_MSI);
> +	if (!msi_add_quirk(dev)) 
> +		printk(KERN_WARNING "PCI: PXH quirk detected, disabling MSI for SHPC device\n");
> +	else {
> +		pci_msi_quirk = 1;
> +		printk(KERN_WARNING "PCI: PXH quirk detected, unable to disable MSI for SHPC device, disabling MSI for all devices\n");
> +	}

Some people use 80-column xterms.   Break the strings up thusly:

		printk(KERN_WARNING "PCI: PXH quirk detected, disabling "
				"MSI for SHPC device\n");

Re: [PATCH] 6700/6702PXH quirk

[Kristen Accardi]

On Fri, 2005-08-05 at 15:26 -0700, Andrew Morton wrote:
> Kristen Accardi <kristen.c.accardi@intel.com> wrote:

> > +	if (!quirk)
> > +		return -ENOMEM;
> > +	
> > +	INIT_LIST_HEAD(&quirk->list);
> > +	quirk->dev = dev;
> > +	list_add(&quirk->list, &msi_quirk_list);
> > +	return 0;
> > +}
> 
> Does the list not need any locking?

Actually, I'm glad you asked that question because I was wondering that
myself.  The devices are added to the list at boot time, and after that
time, the list will never change.  Does PCI enumeration happen on all
processors?  I thought maybe it only happened on one.  In that case we
don't need a lock I don't think.  

Re: [PATCH] 6700/6702PXH quirk

[Jeff Garzik]

AFAICS we don't need a new list, simply consisting of PCI devs.

Just invent, and set, a bit somewhere in struct pci_dev.

	Jeff

Re: [PATCH] 6700/6702PXH quirk

[Andrew Morton]

Kristen Accardi <kristen.c.accardi@intel.com> wrote:
>
> On Fri, 2005-08-05 at 15:26 -0700, Andrew Morton wrote:
> > Kristen Accardi <kristen.c.accardi@intel.com> wrote:
> 
> > > +	if (!quirk)
> > > +		return -ENOMEM;
> > > +	
> > > +	INIT_LIST_HEAD(&quirk->list);
> > > +	quirk->dev = dev;
> > > +	list_add(&quirk->list, &msi_quirk_list);
> > > +	return 0;
> > > +}
> > 
> > Does the list not need any locking?
> 
> Actually, I'm glad you asked that question because I was wondering that
> myself.  The devices are added to the list at boot time, and after that
> time, the list will never change.  Does PCI enumeration happen on all
> processors?  I thought maybe it only happened on one.  In that case we
> don't need a lock I don't think.  
> 

do_basic_setup() is called after SMP is up and running.  do_basic_setup()
calls driver_init() and most of the initcalls.  Plus there's kernel
preemption.

So yup, I think you need locking..

Re: [PATCH] 6700/6702PXH quirk

[Greg KH]

On Fri, Aug 05, 2005 at 03:05:13PM -0700, Kristen Accardi wrote:
> +int msi_add_quirk(struct pci_dev *dev)
> +{
> +	struct msi_quirk *quirk;
> +
> +	quirk = (struct msi_quirk *) kmalloc(sizeof(*quirk), GFP_KERNEL);
> +	if (!quirk)
> +		return -ENOMEM;
> +	
> +	INIT_LIST_HEAD(&quirk->list);
> +	quirk->dev = dev;

You just messed up the reference counting of this device :(

Anyway, Jeff is right, add another bit field.

thanks,

greg k-h

Re: [PATCH] 6700/6702PXH quirk

[Kristen Accardi]

On Fri, 2005-08-05 at 18:50 -0400, Jeff Garzik wrote:
> 
> AFAICS we don't need a new list, simply consisting of PCI devs.
> 
> Just invent, and set, a bit somewhere in struct pci_dev.
> 
> 	Jeff
> 
> 
> 
Great!  I like that much better.  How about this:

On the 6700/6702 PXH part, a MSI may get corrupted if an ACPI hotplug
driver and SHPC driver in MSI mode are used together.  This patch will
prevent MSI from being enabled for the SHPC as part of an early pci
quirk, as well as on any pci device which sets the no_msi bit.  

Signed-off-by: Kristen Carlson Accardi <kristen.c.accardi@intel.com>

diff -uprN -X linux-2.6.13-rc4/Documentation/dontdiff linux-2.6.13-rc4/drivers/pci/msi.c linux-2.6.13-rc4-pxhquirk/drivers/pci/msi.c
--- linux-2.6.13-rc4/drivers/pci/msi.c	2005-07-28 15:44:44.000000000 -0700
+++ linux-2.6.13-rc4-pxhquirk/drivers/pci/msi.c	2005-08-05 16:35:16.000000000 -0700
@@ -453,7 +453,7 @@ static void enable_msi_mode(struct pci_d
 	}
 }
 
-static void disable_msi_mode(struct pci_dev *dev, int pos, int type)
+void disable_msi_mode(struct pci_dev *dev, int pos, int type)
 {
 	u16 control;
 
@@ -699,6 +699,9 @@ int pci_enable_msi(struct pci_dev* dev)
 	if (!pci_msi_enable || !dev)
  		return status;
 
+	if (dev->no_msi)
+		return status;
+
 	temp = dev->irq;
 
 	if ((status = msi_init()) < 0)
diff -uprN -X linux-2.6.13-rc4/Documentation/dontdiff linux-2.6.13-rc4/drivers/pci/pci.h linux-2.6.13-rc4-pxhquirk/drivers/pci/pci.h
--- linux-2.6.13-rc4/drivers/pci/pci.h	2005-07-28 15:44:44.000000000 -0700
+++ linux-2.6.13-rc4-pxhquirk/drivers/pci/pci.h	2005-08-05 16:37:18.000000000 -0700
@@ -46,7 +46,7 @@ extern int pci_msi_quirk;
 #else
 #define pci_msi_quirk 0
 #endif
-
+void disable_msi_mode(struct pci_dev *dev, int pos, int type);
 extern int pcie_mch_quirk;
 extern struct device_attribute pci_dev_attrs[];
 extern struct class_device_attribute class_device_attr_cpuaffinity;
diff -uprN -X linux-2.6.13-rc4/Documentation/dontdiff linux-2.6.13-rc4/drivers/pci/quirks.c linux-2.6.13-rc4-pxhquirk/drivers/pci/quirks.c
--- linux-2.6.13-rc4/drivers/pci/quirks.c	2005-07-28 15:44:44.000000000 -0700
+++ linux-2.6.13-rc4-pxhquirk/drivers/pci/quirks.c	2005-08-05 16:38:28.000000000 -0700
@@ -1267,6 +1267,27 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_IN
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,	PCI_DEVICE_ID_INTEL_E7320_MCH,	quirk_pcie_mch );
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,	PCI_DEVICE_ID_INTEL_E7525_MCH,	quirk_pcie_mch );
 
+
+/* 
+ * It's possible for the MSI to get corrupted if shpc and acpi
+ * are used together on certain PXH-based systems.
+ */
+static void __devinit quirk_pcie_pxh(struct pci_dev *dev)
+{
+	disable_msi_mode(dev, pci_find_capability(dev, PCI_CAP_ID_MSI),
+					PCI_CAP_ID_MSI);
+	dev->no_msi = 1;
+
+	printk(KERN_WARNING "PCI: PXH quirk detected, "
+		"disabling MSI for SHPC device\n");
+}
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL,	PCI_DEVICE_ID_INTEL_PXHD_0,	quirk_pcie_pxh);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL,	PCI_DEVICE_ID_INTEL_PXHD_1,	quirk_pcie_pxh);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL,	PCI_DEVICE_ID_INTEL_PXH_0,	quirk_pcie_pxh);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL,	PCI_DEVICE_ID_INTEL_PXH_1,	quirk_pcie_pxh);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL,	PCI_DEVICE_ID_INTEL_PXHV,	quirk_pcie_pxh);
+
+
 static void __devinit quirk_netmos(struct pci_dev *dev)
 {
 	unsigned int num_parallel = (dev->subsystem_device & 0xf0) >> 4;
diff -uprN -X linux-2.6.13-rc4/Documentation/dontdiff linux-2.6.13-rc4/include/linux/pci.h linux-2.6.13-rc4-pxhquirk/include/linux/pci.h
--- linux-2.6.13-rc4/include/linux/pci.h	2005-07-28 15:44:44.000000000 -0700
+++ linux-2.6.13-rc4-pxhquirk/include/linux/pci.h	2005-08-05 16:37:08.000000000 -0700
@@ -556,6 +556,7 @@ struct pci_dev {
 	/* keep track of device state */
 	unsigned int	is_enabled:1;	/* pci_enable_device has been called */
 	unsigned int	is_busmaster:1; /* device is busmaster */
+	unsigned int	no_msi:1;    	/* device may not use msi */
 	
 	u32		saved_config_space[16]; /* config space saved at suspend time */
 	struct bin_attribute *rom_attr; /* attribute descriptor for sysfs ROM entry */
diff -uprN -X linux-2.6.13-rc4/Documentation/dontdiff linux-2.6.13-rc4/include/linux/pci_ids.h linux-2.6.13-rc4-pxhquirk/include/linux/pci_ids.h
--- linux-2.6.13-rc4/include/linux/pci_ids.h	2005-07-28 15:44:44.000000000 -0700
+++ linux-2.6.13-rc4-pxhquirk/include/linux/pci_ids.h	2005-08-02 13:58:53.000000000 -0700
@@ -2281,6 +2281,11 @@
 #define PCI_VENDOR_ID_INTEL		0x8086
 #define PCI_DEVICE_ID_INTEL_EESSC	0x0008
 #define PCI_DEVICE_ID_INTEL_21145	0x0039
+#define PCI_DEVICE_ID_INTEL_PXHD_0	0x0320
+#define PCI_DEVICE_ID_INTEL_PXHD_1	0x0321
+#define PCI_DEVICE_ID_INTEL_PXH_0	0x0329
+#define PCI_DEVICE_ID_INTEL_PXH_1	0x032A
+#define PCI_DEVICE_ID_INTEL_PXHV	0x032C
 #define PCI_DEVICE_ID_INTEL_82375	0x0482
 #define PCI_DEVICE_ID_INTEL_82424	0x0483
 #define PCI_DEVICE_ID_INTEL_82378	0x0484

Re: [PATCH] 6700/6702PXH quirk

[Jeff Garzik]

On Fri, Aug 05, 2005 at 03:57:12PM -0700, Greg KH wrote:
> Anyway, Jeff is right, add another bit field.


The updated patch, which adds a new bitfield, looks OK to me.

However...

<pedantic>

FWIW, compilers generate AWFUL code for bitfields.  Bitfields are
really tough to do optimally, whereas bit flags ["unsigned int flags &
bitmask"] are the familiar ints and longs that the compiler is well
tuned to optimize.

Additionally, though it is not the case with struct pci_dev, bitfields
cause endian headaches (see the LITTLE_ENDIAN_BITFIELD ifdefs).

Bit flags are -far- superior in every case.  Avoid bitfields like the plague.

</pedantic>

I wouldn't mind seeing a janitor remove all bitfields from struct pci_dev,
and any other kernel structure that uses the evil constructs.

        Jeff

Re: [PATCH] 6700/6702PXH quirk

[Matthew Wilcox]

On Fri, Aug 05, 2005 at 11:34:55PM -0400, Jeff Garzik wrote:
> FWIW, compilers generate AWFUL code for bitfields.  Bitfields are
> really tough to do optimally, whereas bit flags ["unsigned int flags &
> bitmask"] are the familiar ints and longs that the compiler is well
> tuned to optimize.

I'm sure the GCC developers would appreciate a good bug report with a
test-case that generates worse code.  If you don't want to mess with their
bug tracking system, just send me a test case and I'll add it for you.

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

Re: [PATCH] 6700/6702PXH quirk

[Jeff Garzik]

On Sat, Aug 06, 2005 at 09:50:13AM +0100, Matthew Wilcox wrote:
> On Fri, Aug 05, 2005 at 11:34:55PM -0400, Jeff Garzik wrote:
> > FWIW, compilers generate AWFUL code for bitfields.  Bitfields are
> > really tough to do optimally, whereas bit flags ["unsigned int flags &
> > bitmask"] are the familiar ints and longs that the compiler is well
> > tuned to optimize.
> 
> I'm sure the GCC developers would appreciate a good bug report with a
> test-case that generates worse code.  If you don't want to mess with their
> bug tracking system, just send me a test case and I'll add it for you.

Its an order-of-complexity issue.  No matter how hard you try,
bitfields will -always- be tougher to optimize, than machine ints.

Bitfields are weirdly-sized, weirdly-aligned integers.  A simple look at
the generated asm from gcc on ARM or MIPS demonstrates the explosion of
code that can sometimes occur, versus a simple 'and' test of a machine
int and a mask.  x86 is a tiny bit better, but still more expensive to
do bitfields than machine ints.

	Jeff

Re: [PATCH] 6700/6702PXH quirk

[Denis Vlasenko]

On Saturday 06 August 2005 18:57, Jeff Garzik wrote:
> On Sat, Aug 06, 2005 at 09:50:13AM +0100, Matthew Wilcox wrote:
> > On Fri, Aug 05, 2005 at 11:34:55PM -0400, Jeff Garzik wrote:
> > > FWIW, compilers generate AWFUL code for bitfields.  Bitfields are
> > > really tough to do optimally, whereas bit flags ["unsigned int flags &
> > > bitmask"] are the familiar ints and longs that the compiler is well
> > > tuned to optimize.
> > 
> > I'm sure the GCC developers would appreciate a good bug report with a
> > test-case that generates worse code.  If you don't want to mess with their
> > bug tracking system, just send me a test case and I'll add it for you.
> 
> Its an order-of-complexity issue.  No matter how hard you try,
> bitfields will -always- be tougher to optimize, than machine ints.
> 
> Bitfields are weirdly-sized, weirdly-aligned integers.  A simple look at
> the generated asm from gcc on ARM or MIPS demonstrates the explosion of
> code that can sometimes occur, versus a simple 'and' test of a machine
> int and a mask.  x86 is a tiny bit better, but still more expensive to
> do bitfields than machine ints.

But we are talking about one-bit field here:

+       unsigned int    no_msi:1;       /* device may not use msi */

If _this_ isn't optimized nicely into ANDs, ORs, etc, then
bug report is in order and gcc should be fixed.
--
vda

Re: [PATCH] 6700/6702PXH quirk

[Bjorn Helgaas]

On Friday 05 August 2005 5:51 pm, Kristen Accardi wrote:
> On the 6700/6702 PXH part, a MSI may get corrupted if an ACPI hotplug
> driver and SHPC driver in MSI mode are used together.  This patch will
> prevent MSI from being enabled for the SHPC as part of an early pci
> quirk, as well as on any pci device which sets the no_msi bit.  

For mailing list archaeology, I assume this is erratum 15 in the
6700/6702 PXH spec update:
	http://download.intel.com/design/chipsets/specupdt/30270609.pdf

which says

	An MSI is generated by the standard hot-plug controller may
	get corrupted in presence of another ACPI hot-plug driver.
	The ACPI driver performs configuration reads of DWSEL/DWORD
	register in order to determine the hot-plug capability of all
	the ACPI devices.  If the MSI is generated by the Standard
	Hot-Plug Controller (SHPC) in this time period, there is a
	possibility of the MSI getting corrupted.  As a result the
	MSI may not get issued upstream to the MCH.  The above is a
	result of interaction of separate events that are unpredict-
	able.

So what still bugs me about this (and I'm probably just showing my
ignorance here), is that we seem to have two drivers (SHPC and ACPI)
poking at the same hardware.  Why is this?

And where exactly is the ACPI code that is involved?  I see shpc_init()
doing config reads of DWORD_DATA, but I don't see how ACPI is involved.
Is there some AML that's doing the config accesses?  Why would there
be AML if we're using SHPC?

> @@ -699,6 +699,9 @@ int pci_enable_msi(struct pci_dev* dev)
>  	if (!pci_msi_enable || !dev)
>   		return status;
>  
> +	if (dev->no_msi)
> +		return status;
> +

Is there any reason not to fold this into the test above it?

> +static void __devinit quirk_pcie_pxh(struct pci_dev *dev)
> +{
> +	disable_msi_mode(dev, pci_find_capability(dev, PCI_CAP_ID_MSI),
> +					PCI_CAP_ID_MSI);

Is this even needed?  You're doing early fixups, which happen before
any drivers touch the device, so you should only need to disable MSI
if the BIOS can leave it enabled.  But I would have thought MSI would
be disabled until a driver explicitly enables it.

Re: [PATCH] 6700/6702PXH quirk

[Zach Brown]

Jeff Garzik wrote:

> <pedantic>
> 
> FWIW, compilers generate AWFUL code for bitfields.  Bitfields are
> really tough to do optimally, whereas bit flags ["unsigned int flags &
> bitmask"] are the familiar ints and longs that the compiler is well
> tuned to optimize.

I wouldn't have chosen the micro-optimization argument against bitfields
because people who use them as booleans will be more than willing to
trade minuscule performance degredation in non-critical paths for
heaping piles of legibility:

	if (!foo->enabled)
	if (!(foo->flags & FOO_FLAG_ENABLED)

No, I would have chosen the maintenance risk of forgetting that they're
really 1 bit wide scalars which truncate on assignment.

	struct foo {
		unsigned enabled:1;
	};

	int some_thing_is_enabled(thing) {
		return thing->whatever & some_high_bit;
	}

	foo->enabled = some_thing_is_enabled()

Requiring people to remember that they want !!() around assignments
seems much more dangerous.  They'll get left out to "optimize" current
behaviour, leaving land mines in the tree for future maintainers.

- z

Re: [PATCH] 6700/6702PXH quirk

[David S. Miller]

From: Zach Brown <zab@zabbo.net>
Date: Mon, 08 Aug 2005 10:42:37 -0700

> 	if (!foo->enabled)
> 	if (!(foo->flags & FOO_FLAG_ENABLED)

You can hide the "complexity" of the second line behind
macros.  And this is what is done in most places.

Alternatively, you can use the existing bitops interfaces,
and in that case you define bit numbers only then use the
bitops.h interfaces to do all the work (probably the __set_bit()
et al. non-atomic variants in this case).

Really, I think it's worth it.  I absolutely refuse to put sets of
boolean states into C bitfields or even worse integer members.
Just define a u32 bitmask and be done with it.

Re: [PATCH] 6700/6702PXH quirk

[Zach Brown]

> You can hide the "complexity" of the second line behind
> macros.  And this is what is done in most places.

oh, I agree.  My only point is that if the *only* argument against
bitfields is that they're inefficient (insert vague hand-waving) then
people will happily decide to live with that inefficiency.  I'm all for
macros that are both efficient *and* abstract away the risk of getting
open-coding wrong.

- z

Re: [PATCH] 6700/6702PXH quirk

[Kristen Accardi]

On Mon, 2005-08-08 at 10:36 -0600, Bjorn Helgaas wrote:
> On Friday 05 August 2005 5:51 pm, Kristen Accardi wrote:
> > On the 6700/6702 PXH part, a MSI may get corrupted if an ACPI hotplug
> > driver and SHPC driver in MSI mode are used together.  This patch will
> > prevent MSI from being enabled for the SHPC as part of an early pci
> > quirk, as well as on any pci device which sets the no_msi bit.  
> 
> For mailing list archaeology, I assume this is erratum 15 in the
> 6700/6702 PXH spec update:
> 	http://download.intel.com/design/chipsets/specupdt/30270609.pdf
> 
> which says
> 
> 	An MSI is generated by the standard hot-plug controller may
> 	get corrupted in presence of another ACPI hot-plug driver.
> 	The ACPI driver performs configuration reads of DWSEL/DWORD
> 	register in order to determine the hot-plug capability of all
> 	the ACPI devices.  If the MSI is generated by the Standard
> 	Hot-Plug Controller (SHPC) in this time period, there is a
> 	possibility of the MSI getting corrupted.  As a result the
> 	MSI may not get issued upstream to the MCH.  The above is a
> 	result of interaction of separate events that are unpredict-
> 	able.

That's correct.

> 
> So what still bugs me about this (and I'm probably just showing my
> ignorance here), is that we seem to have two drivers (SHPC and ACPI)
> poking at the same hardware.  Why is this?
> 
> And where exactly is the ACPI code that is involved?  I see shpc_init()
> doing config reads of DWORD_DATA, but I don't see how ACPI is involved.
> Is there some AML that's doing the config accesses?  Why would there
> be AML if we're using SHPC?
> 
> > @@ -699,6 +699,9 @@ int pci_enable_msi(struct pci_dev* dev)
> >  	if (!pci_msi_enable || !dev)
> >   		return status;
> >  
> > +	if (dev->no_msi)
> > +		return status;
> > +
> 

I am just learning this stuff as well, so hopefully someone will correct
me if I'm wrong.  This seems like a poorly worded erratum.  The acpiphp
driver does not actual do any config reads - it just asks the acpi core
to read the acpi namespace to determine the hotplug capabilities.  I
will try to find out more about the test case that they used to discover
this problem and get someone to explain it to me in english.


> Is there any reason not to fold this into the test above it?
> 

No, it seems that patches done at 4:45 on Friday don't turn out
optimally :).

> > +static void __devinit quirk_pcie_pxh(struct pci_dev *dev)
> > +{
> > +	disable_msi_mode(dev, pci_find_capability(dev, PCI_CAP_ID_MSI),
> > +					PCI_CAP_ID_MSI);
> 
> Is this even needed?  You're doing early fixups, which happen before
> any drivers touch the device, so you should only need to disable MSI
> if the BIOS can leave it enabled.  But I would have thought MSI would
> be disabled until a driver explicitly enables it.

This was me being paranoid.  I was concerned that some BIOS might decide
to enable by default, so I was just trying to make really really sure
that MSI would be turned off.  Think that's overkill?