[patch 0/7] Improve MSI detection v5

[patch 0/7] Improve MSI detection v5

[Brice Goglin]

Here's another set of patches to cleanup MSI detection to
replace the patchset that is currently in Greg's tree.

After -mm5 breakage due to some root chipsets not having a
subordinate bus, we now use the dev->no_msi flag to disable
MSI on a chipset instead of the subordinate bus flag (see #3).

Other changes include a TTL in the pci_find_next_capability()
loop to find MSI HT cap (see #5), and changing the ipath driver
to use the common PCI_CAP_ID_HT (see #4).

#1 - Merge existing MSI disabling quirks
#2 - Factorize common MSI detection code from pci_enable_msi() and msix()
#3 - Check root chipset no_msi flag instead of all parent busses flags
#4 - Rename PCI_CAP_ID_HT_IRQCONF into PCI_CAP_ID_HT
#5 - Blacklist PCI-E chipsets depending on Hypertransport MSI capability
#6 - Drop pci_msi_quirk
#7 - Drop pci bus_flags

These patches are against 2.6.17-git20.

Brice Goglin

[patch 1/7] Merge existing MSI disabling quirks

[Brice Goglin]

[-- Attachment #1: 01-merge_msi_disabling_quirks.patch --]
[-- Type: text/plain, Size: 2186 bytes --]

Merge existing MSI disabling quirks into a generic one that we will
use to blacklist all MSI-broken chipsets.
By the way, print the bus id of the device.

Signed-off-by: Brice Goglin <brice@myri.com>
---
 drivers/pci/quirks.c |   29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

Index: linux-git/drivers/pci/quirks.c
===================================================================
--- linux-git.orig/drivers/pci/quirks.c	2006-07-02 11:16:08.000000000 -0400
+++ linux-git/drivers/pci/quirks.c	2006-07-02 11:24:07.000000000 -0400
@@ -585,12 +585,6 @@
 { 
         unsigned char revid, tmp;
         
-	if (dev->subordinate) {
-		printk(KERN_WARNING "PCI: MSI quirk detected. "
-		       "PCI_BUS_FLAGS_NO_MSI set for subordinate bus.\n");
-		dev->subordinate->bus_flags |= PCI_BUS_FLAGS_NO_MSI;
-	}
-
         if (nr_ioapics == 0) 
                 return;
 
@@ -603,13 +597,6 @@
         }
 } 
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_8131_BRIDGE, quirk_amd_8131_ioapic);
-
-static void __init quirk_svw_msi(struct pci_dev *dev)
-{
-	pci_msi_quirk = 1;
-	printk(KERN_WARNING "PCI: MSI quirk detected. pci_msi_quirk set.\n");
-}
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_SERVERWORKS, PCI_DEVICE_ID_SERVERWORKS_GCNB_LE, quirk_svw_msi );
 #endif /* CONFIG_X86_IO_APIC */
 
 
@@ -1517,6 +1504,22 @@
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_NVIDIA,  PCI_DEVICE_ID_NVIDIA_CK804_PCIE,
 			quirk_nvidia_ck804_pcie_aer_ext_cap);
 
+#ifdef CONFIG_PCI_MSI
+/* Disable MSI on chipsets that are known to not support it */
+static void __devinit quirk_disable_msi(struct pci_dev *dev)
+{
+	if (dev->subordinate) {
+		printk(KERN_WARNING "PCI: MSI quirk detected. "
+		       "PCI_BUS_FLAGS_NO_MSI set for %s subordinate bus.\n",
+		       pci_name(dev));
+		dev->subordinate->bus_flags |= PCI_BUS_FLAGS_NO_MSI;
+	}
+}
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_8131_BRIDGE, quirk_disable_msi);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_SERVERWORKS, PCI_DEVICE_ID_SERVERWORKS_GCNB_LE,
+			quirk_disable_msi);
+#endif /* CONFIG_PCI_MSI */
+
 EXPORT_SYMBOL(pcie_mch_quirk);
 #ifdef CONFIG_HOTPLUG
 EXPORT_SYMBOL(pci_fixup_device);

[patch 2/7] Factorize common MSI detection code from pci_enable_msi() and msix()

[Brice Goglin]

[-- Attachment #1: 02-factorize_pci_msi_supported.patch --]
[-- Type: text/plain, Size: 2248 bytes --]

pci_enable_msi() and pci_enable_msix() have to check same things
before enabling MSI. Factorize this code in pci_msi_supported().

Signed-off-by: Brice Goglin <brice@myri.com>
---
 drivers/pci/msi.c |   46 ++++++++++++++++++++++++++--------------------
 1 file changed, 26 insertions(+), 20 deletions(-)

Index: linux-git/drivers/pci/msi.c
===================================================================
--- linux-git.orig/drivers/pci/msi.c	2006-07-02 20:39:11.000000000 -0400
+++ linux-git/drivers/pci/msi.c	2006-07-02 20:39:20.000000000 -0400
@@ -901,6 +901,28 @@
 }
 
 /**
+ * pci_msi_supported - check whether MSI may be enabled on device
+ * @dev: pointer to the pci_dev data structure of MSI device function
+ *
+ * MSI must be globally enabled and supported by the device and
+ * its parent busses.
+ **/
+static
+int pci_msi_supported(struct pci_dev * dev)
+{
+	struct pci_bus *bus;
+
+	if (!pci_msi_enable || !dev || dev->no_msi)
+		return -1;
+
+	for (bus = dev->bus; bus; bus = bus->parent)
+		if (bus->bus_flags & PCI_BUS_FLAGS_NO_MSI)
+			return -1;
+
+	return 0;
+}
+
+/**
  * pci_enable_msi - configure device's MSI capability structure
  * @dev: pointer to the pci_dev data structure of MSI device function
  *
@@ -912,19 +934,11 @@
  **/
 int pci_enable_msi(struct pci_dev* dev)
 {
-	struct pci_bus *bus;
-	int pos, temp, status = -EINVAL;
+	int pos, temp, status;
 	u16 control;
 
-	if (!pci_msi_enable || !dev)
- 		return status;
-
-	if (dev->no_msi)
-		return status;
-
-	for (bus = dev->bus; bus; bus = bus->parent)
-		if (bus->bus_flags & PCI_BUS_FLAGS_NO_MSI)
-			return -EINVAL;
+	if (pci_msi_supported(dev) < 0)
+		return -EINVAL;
 
 	temp = dev->irq;
 
@@ -1134,22 +1148,14 @@
  **/
 int pci_enable_msix(struct pci_dev* dev, struct msix_entry *entries, int nvec)
 {
-	struct pci_bus *bus;
 	int status, pos, nr_entries, free_vectors;
 	int i, j, temp;
 	u16 control;
 	unsigned long flags;
 
-	if (!pci_msi_enable || !dev || !entries)
+	if (!entries || pci_msi_supported(dev) < 0)
  		return -EINVAL;
 
-	if (dev->no_msi)
-		return -EINVAL;
-
-	for (bus = dev->bus; bus; bus = bus->parent)
-		if (bus->bus_flags & PCI_BUS_FLAGS_NO_MSI)
-			return -EINVAL;
-
 	status = msi_init();
 	if (status < 0)
 		return status;

[patch 3/7] Check root chipset no_msi flag instead of all parent busses flags

[Brice Goglin]

[-- Attachment #1: 03-use_root_chipset_dev_no_msi_instead_of_pci_bus_flags.patch --]
[-- Type: text/plain, Size: 2378 bytes --]

MSI only requires support in the root chipset, which may not even have
a subordinate bus.
pci_msi_supported() now checks the no_msi flag in the root_chipset pci_dev
struct instead of the PCI_BUS_FLAGS_NO_MSI flag of all its parent busses.
The MSI quirk now sets the no_msi flag accordingly.

Signed-off-by: Brice Goglin <brice@myri.com>
---
 drivers/pci/msi.c    |   15 ++++++++++-----
 drivers/pci/quirks.c |   10 ++++------
 2 files changed, 14 insertions(+), 11 deletions(-)

Index: linux-git/drivers/pci/msi.c
===================================================================
--- linux-git.orig/drivers/pci/msi.c	2006-07-02 20:28:28.000000000 -0400
+++ linux-git/drivers/pci/msi.c	2006-07-02 20:28:58.000000000 -0400
@@ -905,19 +905,24 @@
  * @dev: pointer to the pci_dev data structure of MSI device function
  *
  * MSI must be globally enabled and supported by the device and
- * its parent busses.
+ * its root chipset.
  **/
 static
 int pci_msi_supported(struct pci_dev * dev)
 {
-	struct pci_bus *bus;
+	struct pci_dev *pdev;
 
 	if (!pci_msi_enable || !dev || dev->no_msi)
 		return -1;
 
-	for (bus = dev->bus; bus; bus = bus->parent)
-		if (bus->bus_flags & PCI_BUS_FLAGS_NO_MSI)
-			return -1;
+	/* find root complex for our device */
+	pdev = dev;
+	while (pdev->bus && pdev->bus->self)
+		pdev = pdev->bus->self;
+
+	/* check whether it supports MSI */
+	if (pdev->no_msi)
+		return -1;
 
 	return 0;
 }
Index: linux-git/drivers/pci/quirks.c
===================================================================
--- linux-git.orig/drivers/pci/quirks.c	2006-07-02 20:25:32.000000000 -0400
+++ linux-git/drivers/pci/quirks.c	2006-07-02 20:28:41.000000000 -0400
@@ -1508,12 +1508,10 @@
 /* Disable MSI on chipsets that are known to not support it */
 static void __devinit quirk_disable_msi(struct pci_dev *dev)
 {
-	if (dev->subordinate) {
-		printk(KERN_WARNING "PCI: MSI quirk detected. "
-		       "PCI_BUS_FLAGS_NO_MSI set for %s subordinate bus.\n",
-		       pci_name(dev));
-		dev->subordinate->bus_flags |= PCI_BUS_FLAGS_NO_MSI;
-	}
+	printk(KERN_WARNING "PCI: MSI quirk detected. "
+	       "MSI disabled on chipset %s.\n",
+	       pci_name(dev));
+	dev->no_msi = 1;
 }
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_8131_BRIDGE, quirk_disable_msi);
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_SERVERWORKS, PCI_DEVICE_ID_SERVERWORKS_GCNB_LE,

[patch 4/7] Rename PCI_CAP_ID_HT_IRQCONF into PCI_CAP_ID_HT

[Brice Goglin]

[-- Attachment #1: 04-rename_pci_cap_id_ht_irqconf.patch --]
[-- Type: text/plain, Size: 3118 bytes --]

0x08 is the HT capability, while PCI_CAP_ID_HT_IRQCONF would be
the subtype 0x80 that mpic_scan_ht_pic() uses.
Rename PCI_CAP_ID_HT_IRQCONF into PCI_CAP_ID_HT.

And by the way, use it in the ipath driver instead of defining its
own HT_CAPABILITY_ID.

Signed-off-by: Brice Goglin <brice@myri.com>
---
 arch/powerpc/sysdev/mpic.c                |    2 +-
 drivers/infiniband/hw/ipath/ipath_ht400.c |    5 ++---
 include/linux/pci_regs.h                  |    2 +-
 3 files changed, 4 insertions(+), 5 deletions(-)

Index: linux-git/arch/powerpc/sysdev/mpic.c
===================================================================
--- linux-git.orig/arch/powerpc/sysdev/mpic.c	2006-07-02 11:41:33.000000000 -0400
+++ linux-git/arch/powerpc/sysdev/mpic.c	2006-07-02 11:41:40.000000000 -0400
@@ -249,7 +249,7 @@
 	for (pos = readb(devbase + PCI_CAPABILITY_LIST); pos != 0;
 	     pos = readb(devbase + pos + PCI_CAP_LIST_NEXT)) {
 		u8 id = readb(devbase + pos + PCI_CAP_LIST_ID);
-		if (id == PCI_CAP_ID_HT_IRQCONF) {
+		if (id == PCI_CAP_ID_HT) {
 			id = readb(devbase + pos + 3);
 			if (id == 0x80)
 				break;
Index: linux-git/include/linux/pci_regs.h
===================================================================
--- linux-git.orig/include/linux/pci_regs.h	2006-07-02 11:41:33.000000000 -0400
+++ linux-git/include/linux/pci_regs.h	2006-07-02 11:41:40.000000000 -0400
@@ -196,7 +196,7 @@
 #define  PCI_CAP_ID_MSI		0x05	/* Message Signalled Interrupts */
 #define  PCI_CAP_ID_CHSWP	0x06	/* CompactPCI HotSwap */
 #define  PCI_CAP_ID_PCIX	0x07	/* PCI-X */
-#define  PCI_CAP_ID_HT_IRQCONF	0x08	/* HyperTransport IRQ Configuration */
+#define  PCI_CAP_ID_HT		0x08	/* HyperTransport */
 #define  PCI_CAP_ID_VNDR	0x09	/* Vendor specific capability */
 #define  PCI_CAP_ID_SHPC 	0x0C	/* PCI Standard Hot-Plug Controller */
 #define  PCI_CAP_ID_EXP 	0x10	/* PCI Express */
Index: linux-git/drivers/infiniband/hw/ipath/ipath_ht400.c
===================================================================
--- linux-git.orig/drivers/infiniband/hw/ipath/ipath_ht400.c	2006-07-02 11:42:39.000000000 -0400
+++ linux-git/drivers/infiniband/hw/ipath/ipath_ht400.c	2006-07-02 11:43:09.000000000 -0400
@@ -742,7 +742,6 @@
 	return 0;
 }
 
-#define HT_CAPABILITY_ID   0x08	/* HT capabilities not defined in kernel */
 #define HT_INTR_DISC_CONFIG  0x80	/* HT interrupt and discovery cap */
 #define HT_INTR_REG_INDEX    2	/* intconfig requires indirect accesses */
 
@@ -973,7 +972,7 @@
 	 * do this early, before we ever enable errors or hardware errors,
 	 * mostly to avoid causing the chip to enter freeze mode.
 	 */
-	pos = pci_find_capability(pdev, HT_CAPABILITY_ID);
+	pos = pci_find_capability(pdev, PCI_CAP_ID_HT);
 	if (!pos) {
 		ipath_dev_err(dd, "Couldn't find HyperTransport "
 			      "capability; no interrupts\n");
@@ -996,7 +995,7 @@
 		else if (cap_type == HT_INTR_DISC_CONFIG)
 			ihandler = set_int_handler(dd, pdev, pos);
 	} while ((pos = pci_find_next_capability(pdev, pos,
-						 HT_CAPABILITY_ID)));
+						 PCI_CAP_ID_HT)));
 
 	if (!ihandler) {
 		ipath_dev_err(dd, "Couldn't find interrupt handler in "

[patch 5/7] Blacklist PCI-E chipsets depending on Hypertransport MSI capability

[Brice Goglin]

[-- Attachment #1: 05-check_hypertransport_msi_capabilities.patch --]
[-- Type: text/plain, Size: 3869 bytes --]

Introduce msi_ht_cap_enabled() to check the MSI capability in the
Hypertransport configuration space.
It is used in a generic quirk quirk_msi_ht_cap() to check whether
MSI is enabled on hypertransport chipset, and a nVidia specific quirk
quirk_nvidia_ck804_msi_ht_cap() where two 2 HT MSI mappings have to
be checked.
Both quirks set the no_msi flag when MSI is disabled.

Signed-off-by: Brice Goglin <brice@myri.com>
---
In case of broken hardware, there's now a TTL to protect the
pci_find_next_capability() loop when looking for the HT MSI cap.

 drivers/pci/quirks.c    |   55 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci_ids.h |    1 
 2 files changed, 56 insertions(+)

Index: linux-git/drivers/pci/quirks.c
===================================================================
--- linux-git.orig/drivers/pci/quirks.c	2006-07-02 11:53:09.000000000 -0400
+++ linux-git/drivers/pci/quirks.c	2006-07-02 12:28:49.000000000 -0400
@@ -1516,6 +1516,61 @@
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_8131_BRIDGE, quirk_disable_msi);
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_SERVERWORKS, PCI_DEVICE_ID_SERVERWORKS_GCNB_LE,
 			quirk_disable_msi);
+
+/* Go through the list of Hypertransport capabilities and
+ * return 1 if a HT MSI capability is found and enabled */
+static int __devinit msi_ht_cap_enabled(struct pci_dev *dev)
+{
+	u8 pos;
+	int ttl;
+	for (pos = pci_find_capability(dev, PCI_CAP_ID_HT), ttl = 48;
+	     pos && ttl;
+	     pos = pci_find_next_capability(dev, pos, PCI_CAP_ID_HT), ttl--) {
+		u32 cap_hdr;
+		/* MSI mapping section according to Hypertransport spec */
+		if (pci_read_config_dword(dev, pos, &cap_hdr) == 0
+		    && (cap_hdr & 0xf8000000) == 0xa8000000 /* MSI mapping */) {
+			printk(KERN_INFO "PCI: Found HT MSI mapping on %s with capability %s\n",
+			       pci_name(dev), cap_hdr & 0x10000 ? "enabled" : "disabled");
+			return (cap_hdr & 0x10000) != 0; /* MSI mapping cap enabled */
+		}
+	}
+	return 0;
+}
+
+/* Check the hypertransport MSI mapping to know whether MSI is enabled or not */
+static void __devinit quirk_msi_ht_cap(struct pci_dev *dev)
+{
+	if (!msi_ht_cap_enabled(dev)) {
+		printk(KERN_WARNING "PCI: MSI quirk detected. "
+		       "MSI disabled on chipset %s.\n",
+		       pci_name(dev));
+		dev->no_msi = 1;
+	}
+}
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_SERVERWORKS, PCI_DEVICE_ID_SERVERWORKS_HT2000_PCIE,
+			quirk_msi_ht_cap);
+
+/* The nVidia CK804 chipset may have 2 HT MSI mappings.
+ * MSI are supported if the MSI capability set in any of these mappings.
+ */
+static void __devinit quirk_nvidia_ck804_msi_ht_cap(struct pci_dev *dev)
+{
+	struct pci_dev *pdev;
+
+	/* check HT MSI cap on this chipset and the root one.
+	 * a single one having MSI is enough to be sure that MSI are supported.
+	 */
+	pdev = pci_find_slot(dev->bus->number, 0);
+	if (!msi_ht_cap_enabled(dev) && !msi_ht_cap_enabled(pdev)) {
+		printk(KERN_WARNING "PCI: MSI quirk detected. "
+		       "MSI disabled on chipset %s.\n",
+		       pci_name(dev));
+		dev->no_msi = 1;
+	}
+}
+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);
Index: linux-git/include/linux/pci_ids.h
===================================================================
--- linux-git.orig/include/linux/pci_ids.h	2006-07-02 11:52:37.000000000 -0400
+++ linux-git/include/linux/pci_ids.h	2006-07-02 11:54:01.000000000 -0400
@@ -1407,6 +1407,7 @@
 #define PCI_DEVICE_ID_SERVERWORKS_LE	  0x0009
 #define PCI_DEVICE_ID_SERVERWORKS_GCNB_LE 0x0017
 #define PCI_DEVICE_ID_SERVERWORKS_EPB	  0x0103
+#define PCI_DEVICE_ID_SERVERWORKS_HT2000_PCIE	0x0132
 #define PCI_DEVICE_ID_SERVERWORKS_OSB4	  0x0200
 #define PCI_DEVICE_ID_SERVERWORKS_CSB5	  0x0201
 #define PCI_DEVICE_ID_SERVERWORKS_CSB6    0x0203

[patch 6/7] Drop pci_msi_quirk

[Brice Goglin]

[-- Attachment #1: 06-drop_pci_msi_quirk.patch --]
[-- Type: text/plain, Size: 1921 bytes --]

pci_msi_quirk is not used anymore, the no_msi flag in the pci_dev
structure of the chipset should be preferred.
Drop pci_msi_quirk completely.

Signed-off-by: Brice Goglin <brice@myri.com>
---
 drivers/pci/msi.c    |    7 -------
 drivers/pci/pci.h    |    6 ------
 drivers/pci/quirks.c |    2 --
 3 files changed, 15 deletions(-)

Index: linux-git/drivers/pci/msi.c
===================================================================
--- linux-git.orig/drivers/pci/msi.c	2006-07-02 11:39:56.000000000 -0400
+++ linux-git/drivers/pci/msi.c	2006-07-02 11:56:48.000000000 -0400
@@ -351,13 +351,6 @@
 	if (!status)
 		return status;
 
-	if (pci_msi_quirk) {
-		pci_msi_enable = 0;
-		printk(KERN_WARNING "PCI: MSI quirk detected. MSI disabled.\n");
-		status = -EINVAL;
-		return status;
-	}
-
 	status = msi_arch_init();
 	if (status < 0) {
 		pci_msi_enable = 0;
Index: linux-git/drivers/pci/pci.h
===================================================================
--- linux-git.orig/drivers/pci/pci.h	2006-07-02 11:16:08.000000000 -0400
+++ linux-git/drivers/pci/pci.h	2006-07-02 11:56:48.000000000 -0400
@@ -42,12 +42,6 @@
 /* Lock for read/write access to pci device and bus lists */
 extern struct rw_semaphore pci_bus_sem;
 
-#ifdef CONFIG_X86_IO_APIC
-extern int pci_msi_quirk;
-#else
-#define pci_msi_quirk 0
-#endif
-
 #ifdef CONFIG_PCI_MSI
 void disable_msi_mode(struct pci_dev *dev, int pos, int type);
 void pci_no_msi(void);
Index: linux-git/drivers/pci/quirks.c
===================================================================
--- linux-git.orig/drivers/pci/quirks.c	2006-07-02 11:54:01.000000000 -0400
+++ linux-git/drivers/pci/quirks.c	2006-07-02 11:56:48.000000000 -0400
@@ -575,8 +575,6 @@
 }
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_SI,	PCI_ANY_ID,			quirk_ioapic_rmw );
 
-int pci_msi_quirk;
-
 #define AMD8131_revA0        0x01
 #define AMD8131_revB0        0x11
 #define AMD8131_MISC         0x40

[patch 7/7] Drop pci bus_flags

[Brice Goglin]

[-- Attachment #1: 07-drop_pci_bus_flags.patch --]
[-- Type: text/plain, Size: 1589 bytes --]

PCI_BUS_FLAGS_NO_MSI is not used anymore, and gcc does not like
empty enums. Drop the bus_flags field entirely.

Signed-off-by: Brice Goglin <brice@myri.com>
---
 drivers/pci/probe.c |    1 -
 include/linux/pci.h |    6 ------
 2 files changed, 7 deletions(-)

Index: linux-git/drivers/pci/probe.c
===================================================================
--- linux-git.orig/drivers/pci/probe.c	2006-07-02 11:58:18.000000000 -0400
+++ linux-git/drivers/pci/probe.c	2006-07-02 11:58:41.000000000 -0400
@@ -351,7 +351,6 @@
 	child->parent = parent;
 	child->ops = parent->ops;
 	child->sysdata = parent->sysdata;
-	child->bus_flags = parent->bus_flags;
 	child->bridge = get_device(&bridge->dev);
 
 	child->class_dev.class = &pcibus_class;
Index: linux-git/include/linux/pci.h
===================================================================
--- linux-git.orig/include/linux/pci.h	2006-07-02 11:58:18.000000000 -0400
+++ linux-git/include/linux/pci.h	2006-07-02 12:00:18.000000000 -0400
@@ -94,11 +94,6 @@
 	pci_channel_io_perm_failure = (__force pci_channel_state_t) 3,
 };
 
-typedef unsigned short __bitwise pci_bus_flags_t;
-enum pci_bus_flags {
-	PCI_BUS_FLAGS_NO_MSI = (__force pci_bus_flags_t) 1,
-};
-
 struct pci_cap_saved_state {
 	struct hlist_node next;
 	char cap_nr;
@@ -241,7 +236,6 @@
 	char		name[48];
 
 	unsigned short  bridge_ctl;	/* manage NO_ISA/FBB/et al behaviors */
-	pci_bus_flags_t bus_flags;	/* Inherited by child busses */
 	struct device		*bridge;
 	struct class_device	class_dev;
 	struct bin_attribute	*legacy_io; /* legacy I/O for this bus */

Re: [patch 3/7] Check root chipset no_msi flag instead of all parent busses flags

[Grant Grundler]

On Sun, Jul 02, 2006 at 08:40:02PM -0400, Brice Goglin wrote:
> MSI only requires support in the root chipset, which may not even have
> a subordinate bus.

Brice,
The above comment and the main part of this patch don't look right to me.
Maybe it's the glass of wine I had two hours ago and it's around midnight
now...

If the "root chipset" is not a PCI device and the architecture doesn't
fake it, the root chipset (aka "PCI Host bus controller") is not visible
to PCI subsystem. Some of the arch code (e.g. drivers/parisc/dino.c)
uses "bus->self == NULL" to differentiate between PCI-PCI secondary busses
and PCI bus below a "root chipset". ISTR alpha and sparc doing the same.

Can you confirm I'm remembering/understanding this bit correctly?

> pci_msi_supported() now checks the no_msi flag in the root_chipset pci_dev
> struct instead of the PCI_BUS_FLAGS_NO_MSI flag of all its parent busses.
> The MSI quirk now sets the no_msi flag accordingly.

I don't see how this could work for alpha/parisc/sparc (IIRC) or any other
architecture that doesn't create "fake" PCI Root busses.
I think your previous patch was correct in this regard.

> Index: linux-git/drivers/pci/msi.c
> ===================================================================
> --- linux-git.orig/drivers/pci/msi.c	2006-07-02 20:28:28.000000000 -0400
> +++ linux-git/drivers/pci/msi.c	2006-07-02 20:28:58.000000000 -0400
> @@ -905,19 +905,24 @@
>   * @dev: pointer to the pci_dev data structure of MSI device function
>   *
>   * MSI must be globally enabled and supported by the device and
> - * its parent busses.
> + * its root chipset.
>   **/
>  static
>  int pci_msi_supported(struct pci_dev * dev)
>  {
> -	struct pci_bus *bus;
> +	struct pci_dev *pdev;
>  
>  	if (!pci_msi_enable || !dev || dev->no_msi)
>  		return -1;
>  
> -	for (bus = dev->bus; bus; bus = bus->parent)
> -		if (bus->bus_flags & PCI_BUS_FLAGS_NO_MSI)
> -			return -1;
> +	/* find root complex for our device */
> +	pdev = dev;
> +	while (pdev->bus && pdev->bus->self)
> +		pdev = pdev->bus->self;

In particular, I'm thinking this loop isn't correct though I'm not
in a particular, erm, "certain" frame of mind right now.
Under a parisc root device, pdev->bus->self would be NULL.

Maybe that's why parisc uses pci_scan_bus_parented()
instead of pci_scan_bus(). (I've forgot the background
behind that change).

thanks,
grant

Re: [patch 3/7] Check root chipset no_msi flag instead of all parent busses flags

[Brice Goglin]

Grant Grundler wrote:
> If the "root chipset" is not a PCI device and the architecture doesn't
> fake it, the root chipset (aka "PCI Host bus controller") is not visible
> to PCI subsystem. Some of the arch code (e.g. drivers/parisc/dino.c)
> uses "bus->self == NULL" to differentiate between PCI-PCI secondary busses
> and PCI bus below a "root chipset". ISTR alpha and sparc doing the same.
>
> Can you confirm I'm remembering/understanding this bit correctly?
>   

I am not familiar enough with these architectures, but I guess somebody
else is.

Are MSI working on these architectures? The MSI code in Linux seems to
be very specific so far. And CONFIG_PCI_MSI currently depends on
(X86_LOCAL_APIC && X86_IO_APIC) || IA64

>> pci_msi_supported() now checks the no_msi flag in the root_chipset pci_dev
>> struct instead of the PCI_BUS_FLAGS_NO_MSI flag of all its parent busses.
>> The MSI quirk now sets the no_msi flag accordingly.
>>     
>
> I don't see how this could work for alpha/parisc/sparc (IIRC) or any other
> architecture that doesn't create "fake" PCI Root busses.
> I think your previous patch was correct in this regard.
>   

I don't think it was better. I had the same loop to find the root
chipset. Only the check afterwards has been changed: instead of checking
the subordinate bus flags on the root chipset, I checks its no_msi. Both
patches applied to these architectures would fail to find the root
chipset in the while loop. They will find a bridge right under the root
chipset (or the device itself). The flags are then checked on the bridge
bus, not on the bus that is right under the root chipset.


Anyway, assuming that Linux supports MSI on any of these architectures
and that we find a root chipset that does not support MSI, how would we
blacklist it? There's no way to add a quirk if there is no pci_dev
associated to this root chipset, right?
Assuming we find a place to add some code to disable MSI
(pcibios_fixup_foo?), we would have to set a no_msi flag somewhere. It
might be good to revive the bus flags for this case. But, that's a lot
of "assuming", I'd rather know whether all this is possible first :)

Thanks,
Brice

Re: [patch 3/7] Check root chipset no_msi flag instead of all parent busses flags

[Grant Grundler]

On Tue, Jul 04, 2006 at 07:12:25PM -0400, Brice Goglin wrote:
> Grant Grundler wrote:
> > If the "root chipset" is not a PCI device and the architecture doesn't
> > fake it, the root chipset (aka "PCI Host bus controller") is not visible
> > to PCI subsystem. Some of the arch code (e.g. drivers/parisc/dino.c)
> > uses "bus->self == NULL" to differentiate between PCI-PCI secondary busses
> > and PCI bus below a "root chipset". ISTR alpha and sparc doing the same.
> >
> > Can you confirm I'm remembering/understanding this bit correctly?
> >   
> 
> I am not familiar enough with these architectures, but I guess somebody
> else is.

I've looked at alpha PCI code in the past (never changed it)
and wrote nearly all of the parisc PCI support.

> Are MSI working on these architectures?

Not yet. But MSI support on parisc will be a summer project for me.
Alpha and SPARC also use transaction based interrupts to get
the attention of the CPU. So I expect MSI is possible on them.

>  The MSI code in Linux seems to
> be very specific so far. And CONFIG_PCI_MSI currently depends on
> (X86_LOCAL_APIC && X86_IO_APIC) || IA64

I believe PPC folks are also working on support.

> > I don't see how this could work for alpha/parisc/sparc (IIRC) or any other
> > architecture that doesn't create "fake" PCI Root busses.
> > I think your previous patch was correct in this regard.
> 
> I don't think it was better. I had the same loop to find the root
> chipset. Only the check afterwards has been changed: instead of checking
> the subordinate bus flags on the root chipset, I checks its no_msi. Both
> patches applied to these architectures would fail to find the root
> chipset in the while loop. They will find a bridge right under the root
> chipset (or the device itself). The flags are then checked on the bridge
> bus, not on the bus that is right under the root chipset.

Right - the "bridge bus" check is different than a "PCI Host bus controller"
device check.

> Anyway, assuming that Linux supports MSI on any of these architectures
> and that we find a root chipset that does not support MSI, how would we
> blacklist it? There's no way to add a quirk if there is no pci_dev
> associated to this root chipset, right?

Right. parisc/ia64/et al don't need a generic black list.
It's x86 or x86-64 specific.
At least until the various "x86-like" architectures use the same
chipsets.

The arch support _could_ mark a flag in the "root" struct pci_bus.
We wouldn't (yet) need a quirk on those arches.
I think I might have a better idea below to implement parisc support
that works with the MSI flag in struct pci_dev like you want it.

> Assuming we find a place to add some code to disable MSI
> (pcibios_fixup_foo?),

Yes, pcibios_fixup_bus is normally the first chance for the arch
specific code to mangle the struct pci_bus allocated by generic PCI code.

> we would have to set a no_msi flag somewhere.
> It might be good to revive the bus flags for this case. But, that's a lot
> of "assuming", I'd rather know whether all this is possible first :)

MSI is certainly possible on parisc. I'm pretty sure it's possible
on ppc, alpha and sparc though I don't know details. This has been
discussed before...but I can't find the references.

Maybe the right "somewhere" is for pcibios_fixup_bus() to enable
(or disable) MSI on each real PCI device if/when the arch can
determine MSI in fact works.
I didn't think of this option last night.

I still don't want the generic PCI code to assume a "root"
PCI Host bus controller was found after that loop.

thanks,
grant

Re: [patch 3/7] Check root chipset no_msi flag instead of all parent busses flags

[Brice Goglin]

Grant Grundler wrote:
> I still don't want the generic PCI code to assume a "root"
> PCI Host bus controller was found after that loop.

If I am not mistaken, we can use the following code to check whether we
found a root chipset:
        unsigned cap;
        u16 val;
        u8 ext_type;
        cap = pci_find_capability(pdev, PCI_CAP_ID_EXP);
        if (cap) {
                pci_read_config_word(pdev, cap + PCI_CAP_FLAGS, &val);
                ext_type = (val & PCI_EXP_FLAGS_TYPE) >> 4;
                if (ext_type == PCI_EXP_TYPE_ROOT_PORT)
                        <check no_msi flag>
        }

Brice

Re: [patch 3/7] Check root chipset no_msi flag instead of all parent busses flags

[Benjamin Herrenschmidt]

On Sun, 2006-07-02 at 20:40 -0400, Brice Goglin wrote:
> plain text document attachment
> (03-use_root_chipset_dev_no_msi_instead_of_pci_bus_flags.patch)
> MSI only requires support in the root chipset, which may not even have
> a subordinate bus.
> pci_msi_supported() now checks the no_msi flag in the root_chipset pci_dev
> struct instead of the PCI_BUS_FLAGS_NO_MSI flag of all its parent busses.
> The MSI quirk now sets the no_msi flag accordingly.

I think we shall have msi_ops hanging off struct pci_bus.

That way, MSI support can be "installed" off a given bus and can be
handled differently when necessary. On many platforms, the MSI handling
will actually be different from one bus to the next (the Apple G5 comes
to mind with its PCIe off the northbridge on one side and it's HT<->PCIe
tunnels on the other, using different mecanisms for MSIs, but there are
or will be others).

Ben.

Re: [patch 3/7] Check root chipset no_msi flag instead of all parent busses flags

[Grant Grundler]

On Wed, Jul 05, 2006 at 12:38:27AM -0400, Brice Goglin wrote:
> Grant Grundler wrote:
> > I still don't want the generic PCI code to assume a "root"
> > PCI Host bus controller was found after that loop.
> 
> If I am not mistaken, we can use the following code to check whether we
> found a root chipset:
>         unsigned cap;
>         u16 val;
>         u8 ext_type;
>         cap = pci_find_capability(pdev, PCI_CAP_ID_EXP);

That might work fine on x86 boxen where firmware fakes all devices
to show up in PCI config space. That's not the case on many other
architectures. My whole point was the pdev doesn't exist for
a root chipset on those other arch's.

grant

>         if (cap) {
>                 pci_read_config_word(pdev, cap + PCI_CAP_FLAGS, &val);
>                 ext_type = (val & PCI_EXP_FLAGS_TYPE) >> 4;
>                 if (ext_type == PCI_EXP_TYPE_ROOT_PORT)
>                         <check no_msi flag>
>         }
> 
> Brice

Re: [patch 3/7] Check root chipset no_msi flag instead of all parent busses flags

[Brice Goglin]

Grant Grundler wrote:
> On Wed, Jul 05, 2006 at 12:38:27AM -0400, Brice Goglin wrote:
>   
>> Grant Grundler wrote:
>>     
>>> I still don't want the generic PCI code to assume a "root"
>>> PCI Host bus controller was found after that loop.
>>>       
>> If I am not mistaken, we can use the following code to check whether we
>> found a root chipset:
>>         unsigned cap;
>>         u16 val;
>>         u8 ext_type;
>>         cap = pci_find_capability(pdev, PCI_CAP_ID_EXP);
>>     
>
> That might work fine on x86 boxen where firmware fakes all devices
> to show up in PCI config space. That's not the case on many other
> architectures. My whole point was the pdev doesn't exist for
> a root chipset on those other arch's.
>   

Right, and the whole point of this check is to detect this case: my
while loop cannot find a root chipset on these architectures since there
is no pci_dev. So the while loop will return another pci_dev, either the
device or another bridge. When we detect that this pci_dev has a wrong
exp_type (not PCI_EXP_TYPE_ROOT_PORT), we know it is not a root chipset,
thus we do not check its no_msi flag.

Brice