6700/6702PXH quirk
PATCH 1/1 6700/6702PXH quirk
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.
drivers/pci/msi.c +35 -1
@@ -38,6 +38,32 @@ int vector_irq[NR_VECTORS] = { [0 ... NRu8 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);
drivers/pci/quirks.c +28 -0
@@ -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_INDECLARE_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 ;
include/linux/pci_ids.h +5 -0
@@ -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
Quoted 3 lines · 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.
Can you outline the scenario that causes the corruption? This patch feels like a band-aid over a deeper problem.
Quoted 7 lines · Bjorn Helgaas
Quoted 3 lines · 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.
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.
drivers/pci/msi.c EXPORT_SYMBOL(pci_enable_msi); · line 1163
Quoted 6 lines · Kristen Accardi
drivers/pci/msi.c +2
-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);
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
Quoted 12 lines · Greg KH
Review EXPORT_SYMBOL(pci_enable_msi); · line 1163
Quoted 6 lines · Kristen Accardi
(quoted diff) +2
-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);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?
Quoted 3 lines
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.
drivers/pci/msi.c +33 -1
@@ -38,6 +38,32 @@ int vector_irq[NR_VECTORS] = { [0 ... NRu8 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 )
drivers/pci/quirks.c +28 -0
@@ -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_INDECLARE_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 ;
include/linux/pci_ids.h +5 -0
@@ -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
Quoted 12 lines · Greg KH
Review EXPORT_SYMBOL(pci_enable_msi); · line 1163
Quoted 6 lines · Kristen Accardi
(quoted diff) +2
-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);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?
Quoted 3 lines
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.
drivers/pci/msi.c +33 -1
@@ -38,6 +38,32 @@ int vector_irq[NR_VECTORS] = { [0 ... NRu8 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 )
drivers/pci/quirks.c +28 -0
@@ -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_INDECLARE_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 ;
include/linux/pci_ids.h +5 -0
@@ -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
Quoted 14 lines · 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.
diff -uprN -X linux-2.6.13-rc4/Documentation/dontdiff
linux-2.6.13-rc4/drivers/pci/msi.clinux-2.6.13-rc4-pxhquirk/drivers/pci/msi.c
drivers/pci/msi.c int vector_irq[NR_VECTORS] = { [0 ... NR · line 43
Quoted 6 lines
drivers/pci/msi.c +3
-0@@ -38,6 +38,32 @@ int vector_irq[NR_VECTORS] = { [0 ... NRu8 irq_vector[NR_IRQ_VECTORS ] = {FIRST_DEVICE_VECTOR ,0 };#endif + +LIST_HEAD (msi_quirk_list); +
Can't this have static scope?
drivers/pci/msi.c int vector_irq[NR_VECTORS] = { [0 ... NR · line 48
Quoted 5 lines
drivers/pci/msi.c +5
-0+struct msi_quirk +{ +struct list_head list; +struct pci_dev *dev; +};
We normally do
struct msi_quirk {drivers/pci/msi.c int vector_irq[NR_VECTORS] = { [0 ... NR · line 55
Quoted 6 lines
drivers/pci/msi.c +6
-0+ +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.
drivers/pci/msi.c int vector_irq[NR_VECTORS] = { [0 ... NR · line 63
Quoted 8 lines
drivers/pci/msi.c +8
-0+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?
drivers/pci/quirks.c line 27
Quoted 10 lines
drivers/pci/quirks.c +4
-0@@ -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.
drivers/pci/quirks.c DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_IN · line 1288
Quoted 10 lines
drivers/pci/quirks.c +10
-0+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");
Quoted 12 lines · Andrew Morton
drivers/pci/msi.c int vector_irq[NR_VECTORS] = { [0 ... NR · line 63
Quoted 8 lines · Kristen Accardi
drivers/pci/msi.c +8
-0+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.
Quoted 15 lines · Kristen Accardi
Quoted 12 lines · Andrew Morton
drivers/pci/msi.c int vector_irq[NR_VECTORS] = { [0 ... NR · line 63
Quoted 8 lines · Kristen Accardi
drivers/pci/msi.c +8
-0+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?
Quoted 6 lines
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..
drivers/pci/msi.c int vector_irq[NR_VECTORS] = { [0 ... NR · line 60
Quoted 10 lines · Kristen Accardi
drivers/pci/msi.c +10
-0+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
Quoted 1 line · Greg KH
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
Quoted 4 lines · Jeff Garzik
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.
Signature · 6 lines
"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
Quoted 9 lines · Matthew Wilcox
Quoted 4 lines · Jeff Garzik
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
Quoted 11 lines · Jeff Garzik
Quoted 9 lines · Matthew Wilcox
Quoted 4 lines · Jeff Garzik
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.
Quoted 8 lines
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
Quoted 6 lines · Jeff Garzik
<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
From: Zach Brown <[email protected]>
Date: Mon, 08 Aug 2005 10:42:37 -0700
Quoted 2 lines
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.
Quoted 3 lines
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
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
Quoted 9 lines · 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
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.
drivers/pci/msi.c +4 -1
@@ -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 )
drivers/pci/pci.h +1 -1
@@ -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;
drivers/pci/quirks.c +21 -0
@@ -1267,6 +1267,27 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INDECLARE_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 ;
include/linux/pci.h +1 -0
@@ -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 */
include/linux/pci_ids.h +5 -0
@@ -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
Quoted 4 lines · 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 as part of an early pci quirk, as well as on any pci device which sets the
no_msibit.
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?
drivers/pci/msi.c int pci_enable_msi(struct pci_dev* dev) · line 704
Quoted 7 lines
drivers/pci/msi.c +3
-0@@ -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?
drivers/pci/quirks.c DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_IN · line 1278
Quoted 4 lines
drivers/pci/quirks.c +4
-0+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.
Quoted 12 lines · Bjorn Helgaas
Quoted 4 lines · 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 as part of an early pci quirk, as well as on any pci device which sets the
no_msibit.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.pdfwhich says
Quoted 10 lines
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.
Quoted 10 lines
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 ofDWORD_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?
Quoted 8 lines
drivers/pci/msi.c int pci_enable_msi(struct pci_dev* dev) · line 704
Quoted 7 lines
drivers/pci/msi.c +3
-0@@ -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.
Quoted 2 lines
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 :).
Quoted 9 lines
drivers/pci/quirks.c DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_IN · line 1278
Quoted 4 lines
drivers/pci/quirks.c +4
-0+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?