PCI: remove access to pci_[enable|disable]_msi() for drivers

Ok, as it seems there is a bit of confusion, here's real code that should help explain what I am proposing. This works on my desktop, but I don't think it supports MSI :)

I'll go dig out an old 4-way AMD box that has MSI to see if this still works properly, but comments are welcome.

thanks,

greg k-h

--------------------

PCI: remove access to pci_[enable|disable]_msi() for drivers

This is now handled by the PCI core. A new function, pci_in_msi_mode() can be used to tell if a PCI device has MSI enabled for it or not.

---
 drivers/infiniband/hw/mthca/mthca_main.c |    6 +-----
 drivers/net/bnx2.c                       |    2 +-
 drivers/net/e1000/e1000_main.c           |    9 ++-------
 drivers/net/ixgb/ixgb_main.c             |   10 +++-------
 drivers/net/tg3.c                        |   19 +------------------
 drivers/pci/hotplug/pciehp_hpc.c         |    2 --
 drivers/pci/hotplug/shpchp_hpc.c         |    4 +---
 drivers/pci/msi.c                        |   29 +++++++++++++++++++++++++++--
 drivers/pci/pci.c                        |    4 +++-
 drivers/pci/pci.h                        |    8 ++++++++
 drivers/pci/pcie/portdrv_core.c          |    5 +----
 include/linux/pci.h                      |    6 ++----
 12 files changed, 50 insertions(+), 54 deletions(-)

@@ -692,6 +692,31 @@
 }
 
 /**
+ * pci_in_msi_mode - determine if a specific PCI device is in MSI mode or not.
+ * @dev: pointer to the pci_dev data structure to test
+ *
+ * Returns 1 if MSI is enabled for this PCI device, otherwise will return 0.
+ **/
+int pci_in_msi_mode(struct pci_dev* dev)
+{
+	int pos;
+	u16 control;
+
+	if (!pci_msi_enable || !dev)
+ 		return 0;
+
+   	if (!(pos = pci_find_capability(dev, PCI_CAP_ID_MSI)))
+		return 0;
+
+	pci_read_config_word(dev, msi_control_reg(pos), &control);
+	if (control & PCI_MSI_FLAGS_ENABLE)
+		return 1;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pci_in_msi_mode);
+
+/**
  * pci_enable_msi - configure device's MSI capability structure
  * @dev: pointer to the pci_dev data structure of MSI device function
  *

@@ -1145,7 +1170,7 @@
 	}
 }
 
-EXPORT_SYMBOL(pci_enable_msi);
-EXPORT_SYMBOL(pci_disable_msi);
+//EXPORT_SYMBOL(pci_enable_msi);
+//EXPORT_SYMBOL(pci_disable_msi);
 EXPORT_SYMBOL(pci_enable_msix);
 EXPORT_SYMBOL(pci_disable_msix);

@@ -880,17 +880,15 @@
 };
 
 #ifndef CONFIG_PCI_MSI
+static inline int pci_in_msi_mode(struct pci_dev* dev) {return 0;}
 static inline void pci_scan_msi_device(struct pci_dev *dev) {}
-static inline int pci_enable_msi(struct pci_dev *dev) {return -1;}
-static inline void pci_disable_msi(struct pci_dev *dev) {}
 static inline int pci_enable_msix(struct pci_dev* dev,
 	struct msix_entry *entries, int nvec) {return -1;}
 static inline void pci_disable_msix(struct pci_dev *dev) {}
 static inline void msi_remove_pci_irq_vectors(struct pci_dev *dev) {}
 #else
+extern int pci_in_msi_mode(struct pci_dev* dev);
 extern void pci_scan_msi_device(struct pci_dev *dev);
-extern int pci_enable_msi(struct pci_dev *dev);
-extern void pci_disable_msi(struct pci_dev *dev);
 extern int pci_enable_msix(struct pci_dev* dev,
 	struct msix_entry *entries, int nvec);
 extern void pci_disable_msix(struct pci_dev *dev);

@@ -402,6 +402,7 @@
 	if ((err = pci_enable_device_bars(dev, (1 << PCI_NUM_RESOURCES) - 1)))
 		return err;
 	pci_fixup_device(pci_fixup_enable, dev);
+	pci_enable_msi(dev);
 	dev->is_enabled = 1;
 	return 0;
 }

@@ -427,7 +428,8 @@
 pci_disable_device(struct pci_dev *dev)
 {
 	u16 pci_command;
-	
+
+	pci_disable_msi(dev);
 	pci_read_config_word(dev, PCI_COMMAND, &pci_command);
 	if (pci_command & PCI_COMMAND_MASTER) {
 		pci_command &= ~PCI_COMMAND_MASTER;

@@ -3888,7 +3888,7 @@
 		(CHIP_ID(bp) != CHIP_ID_5706_A1) &&
 		!disable_msi) {
 
-		if (pci_enable_msi(bp->pdev) == 0) {
+		if (pci_in_msi_mode(bp->pdev)) {
 			bp->flags |= USING_MSI_FLAG;
 			rc = request_irq(bp->pdev->irq, bnx2_msi, 0, dev->name,
 					dev);

@@ -37,6 +37,14 @@
 /* Lock for read/write access to pci device and bus lists */
 extern spinlock_t pci_bus_lock;
 
+#ifndef CONFIG_PCI_MSI
+static inline int pci_enable_msi(struct pci_dev *dev) {return -1;}
+static inline void pci_disable_msi(struct pci_dev *dev) {}
+#else
+extern int pci_enable_msi(struct pci_dev *dev);
+extern void pci_disable_msi(struct pci_dev *dev);
+#endif
+
 #ifdef CONFIG_X86_IO_APIC
 extern int pci_msi_quirk;
 #else

@@ -792,7 +792,6 @@
 		if (php_ctlr->irq) {
 			free_irq(php_ctlr->irq, ctrl);
 			php_ctlr->irq = 0;
-			pci_disable_msi(php_ctlr->pci_dev);
 		}
 	}
 	if (php_ctlr->pci_dev) {

@@ -1552,8 +1551,7 @@
 		start_int_poll_timer( php_ctlr, 10 );   /* start with 10 second delay */
 	} else {
 		/* Installs the interrupt handler */
-		rc = pci_enable_msi(pdev);
-		if (rc) {
+		if (!pci_in_msi_mode(pdev)) {
 			info("Can't get msi for the hotplug controller\n");
 			info("Use INTx for the hotplug controller\n");
 			dbg("%s: rc = %x\n", __FUNCTION__, rc);

@@ -164,8 +164,7 @@
 		pos = pci_find_capability(dev, PCI_CAP_ID_MSI);
 		if (pos) {
 			printk("%s Found MSI capability\n", __FUNCTION__);
-			status = pci_enable_msi(dev);
-			if (!status) {
+			if (pci_in_msi_mode(dev)) {
 				interrupt_mode = PCIE_PORT_MSI_MODE;
 				for (i = 0;i < PCIE_PORT_DEVICE_MAXSERVICES;i++)
 					vectors[i] = dev->irq;

@@ -398,8 +397,6 @@
 	/* Switch to INTx by default if MSI enabled */
 	if (interrupt_mode == PCIE_PORT_MSIX_MODE)
 		pci_disable_msix(dev);
-	else if (interrupt_mode == PCIE_PORT_MSI_MODE)
-		pci_disable_msi(dev);
 }
 
 void pcie_port_bus_register(void)

@@ -321,9 +321,9 @@
 #ifdef CONFIG_PCI_MSI
 	if(adapter->hw.mac_type > e1000_82547_rev_2) {
 		adapter->have_msi = TRUE;
-		if((err = pci_enable_msi(adapter->pdev))) {
+		if (!pci_in_msi_mode(adapter->pdev)) {
 			DPRINTK(PROBE, ERR,
-			 "Unable to allocate MSI interrupt Error: %d\n", err);
+			 "Unable to allocate MSI interrupt\n");
 			adapter->have_msi = FALSE;
 		}
 	}

@@ -353,11 +353,6 @@
 
 	e1000_irq_disable(adapter);
 	free_irq(adapter->pdev->irq, netdev);
-#ifdef CONFIG_PCI_MSI
-	if(adapter->hw.mac_type > e1000_82547_rev_2 &&
-	   adapter->have_msi == TRUE)
-		pci_disable_msi(adapter->pdev);
-#endif
 	del_timer_sync(&adapter->tx_fifo_stall_timer);
 	del_timer_sync(&adapter->watchdog_timer);
 	del_timer_sync(&adapter->phy_info_timer);

@@ -239,9 +239,9 @@
 
 	if (!pcix)
 	   adapter->have_msi = FALSE;
-	else if((err = pci_enable_msi(adapter->pdev))) {
-		printk (KERN_ERR
-		 "Unable to allocate MSI interrupt Error: %d\n", err);
+	else if (!pci_in_msi_mode(adapter->pdev)) {
+		dev_err(&adapter->pdev->dev,
+			"Unable to allocate MSI interrupt\n");
 		adapter->have_msi = FALSE;
 		/* proceed to try to request regular interrupt */
 	}

@@ -291,11 +291,7 @@
 
 	ixgb_irq_disable(adapter);
 	free_irq(adapter->pdev->irq, netdev);
-#ifdef CONFIG_PCI_MSI
-	if(adapter->have_msi == TRUE)
-		pci_disable_msi(adapter->pdev);
 
-#endif
 	if(kill_watchdog)
 		del_timer_sync(&adapter->watchdog_timer);
 #ifdef CONFIG_IXGB_NAPI

@@ -5984,7 +5984,6 @@
 		       tp->dev->name);
 
 	free_irq(tp->pdev->irq, dev);
-	pci_disable_msi(tp->pdev);
 
 	tp->tg3_flags2 &= ~TG3_FLG2_USING_MSI;
 

@@ -6047,7 +6046,7 @@
 		if (!(tp->tg3_flags & TG3_FLAG_TAGGED_STATUS)) {
 			printk(KERN_WARNING PFX "%s: MSI without TAGGED? "
 			       "Not using MSI.\n", tp->dev->name);
-		} else if (pci_enable_msi(tp->pdev) == 0) {
+		} else if (pci_in_msi_mode(tp->pdev)) {
 			u32 msi_mode;
 
 			msi_mode = tr32(MSGINT_MODE);

@@ -6068,10 +6067,6 @@
 	}
 
 	if (err) {
-		if (tp->tg3_flags2 & TG3_FLG2_USING_MSI) {
-			pci_disable_msi(tp->pdev);
-			tp->tg3_flags2 &= ~TG3_FLG2_USING_MSI;
-		}
 		tg3_free_consistent(tp);
 		return err;
 	}

@@ -6106,10 +6101,6 @@
 
 	if (err) {
 		free_irq(tp->pdev->irq, dev);
-		if (tp->tg3_flags2 & TG3_FLG2_USING_MSI) {
-			pci_disable_msi(tp->pdev);
-			tp->tg3_flags2 &= ~TG3_FLG2_USING_MSI;
-		}
 		tg3_free_consistent(tp);
 		return err;
 	}

@@ -6121,10 +6112,6 @@
 			spin_lock_irq(&tp->lock);
 			spin_lock(&tp->tx_lock);
 
-			if (tp->tg3_flags2 & TG3_FLG2_USING_MSI) {
-				pci_disable_msi(tp->pdev);
-				tp->tg3_flags2 &= ~TG3_FLG2_USING_MSI;
-			}
 			tg3_halt(tp, RESET_KIND_SHUTDOWN, 1);
 			tg3_free_rings(tp);
 			tg3_free_consistent(tp);

@@ -6409,10 +6396,6 @@
 	spin_unlock_irq(&tp->lock);
 
 	free_irq(tp->pdev->irq, dev);
-	if (tp->tg3_flags2 & TG3_FLG2_USING_MSI) {
-		pci_disable_msi(tp->pdev);
-		tp->tg3_flags2 &= ~TG3_FLG2_USING_MSI;
-	}
 
 	memcpy(&tp->net_stats_prev, tg3_get_stats(tp->dev),
 	       sizeof(tp->net_stats_prev));

@@ -1001,7 +1001,7 @@
 	if (msi_x && !mthca_enable_msi_x(mdev))
 		mdev->mthca_flags |= MTHCA_FLAG_MSI_X;
 	if (msi && !(mdev->mthca_flags & MTHCA_FLAG_MSI_X) &&
-	    !pci_enable_msi(pdev))
+	    pci_in_msi_mode(pdev))
 		mdev->mthca_flags |= MTHCA_FLAG_MSI;
 
 	sema_init(&mdev->cmd.hcr_sem, 1);

@@ -1076,8 +1076,6 @@
 err_free_dev:
 	if (mdev->mthca_flags & MTHCA_FLAG_MSI_X)
 		pci_disable_msix(pdev);
-	if (mdev->mthca_flags & MTHCA_FLAG_MSI)
-		pci_disable_msi(pdev);
 
 	ib_dealloc_device(&mdev->ib_dev);
 

@@ -1125,8 +1123,6 @@
 
 		if (mdev->mthca_flags & MTHCA_FLAG_MSI_X)
 			pci_disable_msix(pdev);
-		if (mdev->mthca_flags & MTHCA_FLAG_MSI)
-			pci_disable_msi(pdev);
 
 		ib_dealloc_device(&mdev->ib_dev);
 		mthca_release_regions(pdev, mdev->mthca_flags &

@@ -741,8 +741,6 @@
 		if (php_ctlr->irq) {
 			free_irq(php_ctlr->irq, ctrl);
 			php_ctlr->irq = 0;
-			if (!pcie_mch_quirk) 
-				pci_disable_msi(php_ctlr->pci_dev);
 		}
 	}
 	if (php_ctlr->pci_dev) 

Thanks for posting some sample code. Some comments though:

* What if the driver writer does not want MSI enabled for their hardware (even though there is an MSI capabilities entry)? Reasons
include: overhead involved in initiating the MSI; no support in some versions of firmware (QLogic hardware).

* A device (notably, our 4gb PCIe fibre-channel products) can support both MSI and MSI-X. Since the driver has no way of 'disabling' MSI, how would it enable MSI-X?

Thanks,
Andrew

Yes, a very good point. I guess I should keep the pci_enable_msi() and pci_disable_msi() functions exported for this reason.

Agreed, let me respin the patches again, because I think I got the logic wrong on some of these drivers because of this...

thanks,

greg k-h

well... only pci_disable_msi() is needed for this ;)

It depends on how the switch from MSI to MSI-X is supposed to work. I'm thinking of the following sort of sequence:

	pci_disable_msi(dev);
	if (pci_enable_msix(dev,...)) {
		register MSIX stuff
	} else {
		/* MSIX won't work...fall back to MSI */
		pci_enable_msi(dev);
	}

and that makes a bad assumption MSI will work.

grant

I thought so too, until I looked at the IB driver :(

The issue is, if pci_enable_msix() fails, we want to fall back to MSI, so you need to call pci_enable_msi() for that (after calling pci_disable_msi() before calling pci_enable_msix(), what a mess...)

So we still need both functions, and for MSI-X, the logic involved in enabling it is horrible. Let me see if this can be made saner...

thanks,

greg k-h

if the core enables msi.. shouldn't the core also do msix ?

Damm, I knew someone was going to say that :)

Yeah, problem is, the driver provides the msix entries for the core to fill in. I could just move that to the pci structure itself if no one minds, and just use the max number of entries as the default.

Hm, or does it really help for the driver to specify different numbers of msi-x vectors?

Roland, any ideas here? You seem to be the only one who has actually used the msix code so far.

thanks,

greg k-h

Greg> Hm, or does it really help for the driver to specify Greg> different numbers of msi-x vectors?

Greg> Roland, any ideas here? You seem to be the only one who has Greg> actually used the msix code so far.

Yes, I think that the driver definitely needs to be in control of how many MSI-X interrupts it gets. The current mthca driver knows that it has three different event queues -- one for firmware command events, one for async events such as link up/down, and one for actual tx/rx completions -- and uses a separate MSI-X message for each one.

Pretty soon I'm going to want to split the tx/rx event queue into one event queue per CPU, so that applications can have their completions events delivered to the same CPU where they're running, so I'll want to bump up the number of MSI-X entries I request.

I'm sure other MSI-X capable devices can slice and dice things differently.

- R.

Yes. Need to talk to Neterion about MSI-X support for their 10Gige product. I expect Intel's 10Gige also can make use of MSI-X.

One of the key things MSI-X was expected to do was distribute seperate TX/RX "queues" (aka descriptor rings) across CPUs. As such, each CPU would "own" a set of queues.

I don't know if the 10GigE cards were in fact able to implement this functionality. But I would like to know if they did/do.

thanks
grant

The idea was always to have MSI by default and if the driver wants MSI-X it turns off standard MSI and does its own thing completely.

-Andi

It is messy in that case, but still preferable to having MSI code in every driver. I suppose most devices will not use MSI-X for some time...

-Andi

I was going to argue the opposite:
New devices are implementing MSI-X, not MSI. e.g. IB/10GigE implement MSI-X.

But wording in the PCI-[XE] specs forecast what Andi said:

| 2.1.9.    Message-Signaled Interrupts
| PCI-X devices are required to support message-signaled interrupts and
| must support a 64-bit message address, as specified in PCI 2.2.

PCI-E 1.0 has similar language:

| 6.1.4.         Message Signaled Interrupt (MSI) Support
| The Message Signaled Interrupt (MSI) capability is defined in the
| PCI 2.3 Specification. MSI interrupt support, which is optional for
| PCI 2.3 devices, is required for PCI Express devices.  MSI-capable
| devices deliver interrupts by performing memory write transactions.
| MSI is an edge-triggered interrupt; neither the PCI 2.3 Specification
| nor this specification support level-triggered MSI interrupts.

PCI 3.0 spec was the first to mention anything about MSI-X.

PCI-E 1.1 also mentions MSI-X:

| 6.1.4.           Message Signaled Interrupt (MSI/MSI-X) Support
| MSI/MSI-X interrupt support, which is optional for PCI 3.0 devices,
| is required for PCI Express devices. All PCI Express devices that
| are capable of generating interrupts must support MSI or MSI-X or both.
| The MSI and MSI-X mechanisms deliver interrupts by performing memory
|  write transactions. MSI and MSI-X are edge-triggered interrupt
| mechanisms; neither the PCI Local Bus Specification, Revision 3.0 nor
| this specification support level-triggered MSI/MSI-X interrupts.

My point is that MSI-X is optional (an alternative to MSI).

hth,
grant

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Greg.

Why not make pci_switch_to_msix() (yeah, horrible name) instead?

pci_switch_to_msix(dev)
{
pci_disable_msi(dev);
if (!psi_enable_msix(dev))

    pci_enable_msi(dev);
}

And it can naturally inform the caller if it failed or not.

// Stefan
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (MingW32)

iD8DBQFCp8uHBrn2kJu9P78RAid6AKCqYFJM0+kg0JYIgOYQiHSHv5Cw0gCfQw8w A+z+BYzzvfi4oaBl6isuaEM=
=qL+F
-----END PGP SIGNATURE-----

Yes, that would work, if you want to go down that path :)

After trying this all out, I'm convinced that we should just stick with what we have.

thanks,

greg k-h

Heh, yes. As to your other comments, you're right, I got the logic wrong in places. A new patch will be out in a few hours...

thanks,

greg k-h

Hm, here's an updated patch that should have fixed the errors I had in my previous one where I wasn't disabling MSI for the devices that did not want it enabled (note, my patch skips the hotplug and pcie driver for now, those would have to be fixed if this patch goes on.)

However, now that I've messed around with the MSI-X logic in the IB driver, I'm thinking that this whole thing is just pointless, and I should just drop it and we should stick with the current way of enabling MSI only if the driver wants it. If you look at the logic in the mthca driver you'll see what I mean.

So, anyone else think this is a good idea? Votes for me to just drop it and go back to hacking on the driver core instead?

Oh, and in looking at the drivers/pci/msi.c file, it could use some cleanups to make it smaller and a bit less complex. I've also seen some complaints that it is very arch specific (x86 based). But as no other arches seem to want to support MSI, I don't really see any need to split it up. Any comments about this?

thanks,

greg k-h

-----------

PCI: remove access to pci_[enable|disable]_msi() for drivers

This is now handled by the PCI core. A new function, pci_in_msi_mode() can be used to tell if a PCI device has MSI enabled for it or not.

---
 drivers/infiniband/hw/mthca/mthca_main.c |    8 +++-----
 drivers/net/bnx2.c                       |    3 ++-
 drivers/net/e1000/e1000_main.c           |   12 ++++--------
 drivers/net/ixgb/ixgb_main.c             |   15 ++++++---------
 drivers/net/tg3.c                        |   20 ++------------------
 drivers/pci/msi.c                        |   25 +++++++++++++++++++++++++
 drivers/pci/pci.c                        |    4 +++-
 include/linux/pci.h                      |    2 ++

@@ -692,6 +692,31 @@
 }
 
 /**
+ * pci_in_msi_mode - determine if a specific PCI device is in MSI mode or not.
+ * @dev: pointer to the pci_dev data structure to test
+ *
+ * Returns 1 if MSI is enabled for this PCI device, otherwise will return 0.
+ **/
+int pci_in_msi_mode(struct pci_dev* dev)
+{
+	int pos;
+	u16 control;
+
+	if (!pci_msi_enable || !dev)
+ 		return 0;
+
+   	if (!(pos = pci_find_capability(dev, PCI_CAP_ID_MSI)))
+		return 0;
+
+	pci_read_config_word(dev, msi_control_reg(pos), &control);
+	if (control & PCI_MSI_FLAGS_ENABLE)
+		return 1;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pci_in_msi_mode);
+
+/**
  * pci_enable_msi - configure device's MSI capability structure
  * @dev: pointer to the pci_dev data structure of MSI device function
  *

@@ -880,6 +880,7 @@
 };
 
 #ifndef CONFIG_PCI_MSI
+static inline int pci_in_msi_mode(struct pci_dev* dev) {return 0;}
 static inline void pci_scan_msi_device(struct pci_dev *dev) {}
 static inline int pci_enable_msi(struct pci_dev *dev) {return -1;}
 static inline void pci_disable_msi(struct pci_dev *dev) {}

@@ -888,6 +889,7 @@
 static inline void pci_disable_msix(struct pci_dev *dev) {}
 static inline void msi_remove_pci_irq_vectors(struct pci_dev *dev) {}
 #else
+extern int pci_in_msi_mode(struct pci_dev* dev);
 extern void pci_scan_msi_device(struct pci_dev *dev);
 extern int pci_enable_msi(struct pci_dev *dev);
 extern void pci_disable_msi(struct pci_dev *dev);

@@ -402,6 +402,7 @@
 	if ((err = pci_enable_device_bars(dev, (1 << PCI_NUM_RESOURCES) - 1)))
 		return err;
 	pci_fixup_device(pci_fixup_enable, dev);
+	pci_enable_msi(dev);
 	dev->is_enabled = 1;
 	return 0;
 }

@@ -427,7 +428,8 @@
 pci_disable_device(struct pci_dev *dev)
 {
 	u16 pci_command;
-	
+
+	pci_disable_msi(dev);
 	pci_read_config_word(dev, PCI_COMMAND, &pci_command);
 	if (pci_command & PCI_COMMAND_MASTER) {
 		pci_command &= ~PCI_COMMAND_MASTER;

@@ -3888,7 +3888,7 @@
 		(CHIP_ID(bp) != CHIP_ID_5706_A1) &&
 		!disable_msi) {
 
-		if (pci_enable_msi(bp->pdev) == 0) {
+		if (pci_in_msi_mode(bp->pdev)) {
 			bp->flags |= USING_MSI_FLAG;
 			rc = request_irq(bp->pdev->irq, bnx2_msi, 0, dev->name,
 					dev);

@@ -3899,6 +3899,7 @@
 		}
 	}
 	else {
+		pci_disable_msi(bp->pdev);
 		rc = request_irq(bp->pdev->irq, bnx2_interrupt, SA_SHIRQ,
 				dev->name, dev);
 	}

@@ -321,12 +321,13 @@
 #ifdef CONFIG_PCI_MSI
 	if(adapter->hw.mac_type > e1000_82547_rev_2) {
 		adapter->have_msi = TRUE;
-		if((err = pci_enable_msi(adapter->pdev))) {
+		if (!pci_in_msi_mode(adapter->pdev)) {
 			DPRINTK(PROBE, ERR,
-			 "Unable to allocate MSI interrupt Error: %d\n", err);
+			 "Unable to allocate MSI interrupt\n");
 			adapter->have_msi = FALSE;
 		}
-	}
+	} else
+		pci_disable_msi(adapter->pdev);
 #endif
 	if((err = request_irq(adapter->pdev->irq, &e1000_intr,
 		              SA_SHIRQ | SA_SAMPLE_RANDOM,

@@ -353,11 +354,6 @@
 
 	e1000_irq_disable(adapter);
 	free_irq(adapter->pdev->irq, netdev);
-#ifdef CONFIG_PCI_MSI
-	if(adapter->hw.mac_type > e1000_82547_rev_2 &&
-	   adapter->have_msi == TRUE)
-		pci_disable_msi(adapter->pdev);
-#endif
 	del_timer_sync(&adapter->tx_fifo_stall_timer);
 	del_timer_sync(&adapter->watchdog_timer);
 	del_timer_sync(&adapter->phy_info_timer);

@@ -237,11 +237,12 @@
 						  IXGB_STATUS_PCIX_MODE) ? TRUE : FALSE;
 	adapter->have_msi = TRUE;
 
-	if (!pcix)
-	   adapter->have_msi = FALSE;
-	else if((err = pci_enable_msi(adapter->pdev))) {
-		printk (KERN_ERR
-		 "Unable to allocate MSI interrupt Error: %d\n", err);
+	if (!pcix) {
+		adapter->have_msi = FALSE;
+		pci_disable_msi(adapter->pdev);
+	} else if (!pci_in_msi_mode(adapter->pdev)) {
+		dev_err(&adapter->pdev->dev,
+			"Unable to allocate MSI interrupt\n");
 		adapter->have_msi = FALSE;
 		/* proceed to try to request regular interrupt */
 	}

@@ -291,11 +292,7 @@
 
 	ixgb_irq_disable(adapter);
 	free_irq(adapter->pdev->irq, netdev);
-#ifdef CONFIG_PCI_MSI
-	if(adapter->have_msi == TRUE)
-		pci_disable_msi(adapter->pdev);
 
-#endif
 	if(kill_watchdog)
 		del_timer_sync(&adapter->watchdog_timer);
 #ifdef CONFIG_IXGB_NAPI

@@ -5984,7 +5984,6 @@
 		       tp->dev->name);
 
 	free_irq(tp->pdev->irq, dev);
-	pci_disable_msi(tp->pdev);
 
 	tp->tg3_flags2 &= ~TG3_FLG2_USING_MSI;
 

@@ -6047,7 +6046,7 @@
 		if (!(tp->tg3_flags & TG3_FLAG_TAGGED_STATUS)) {
 			printk(KERN_WARNING PFX "%s: MSI without TAGGED? "
 			       "Not using MSI.\n", tp->dev->name);
-		} else if (pci_enable_msi(tp->pdev) == 0) {
+		} else if (pci_in_msi_mode(tp->pdev)) {
 			u32 msi_mode;
 
 			msi_mode = tr32(MSGINT_MODE);

@@ -6063,15 +6062,12 @@
 		if (tp->tg3_flags & TG3_FLAG_TAGGED_STATUS)
 			fn = tg3_interrupt_tagged;
 
+		pci_disable_msi(tp->pdev);
 		err = request_irq(tp->pdev->irq, fn,
 				  SA_SHIRQ | SA_SAMPLE_RANDOM, dev->name, dev);
 	}
 
 	if (err) {
-		if (tp->tg3_flags2 & TG3_FLG2_USING_MSI) {
-			pci_disable_msi(tp->pdev);
-			tp->tg3_flags2 &= ~TG3_FLG2_USING_MSI;
-		}
 		tg3_free_consistent(tp);
 		return err;
 	}

@@ -6106,10 +6102,6 @@
 
 	if (err) {
 		free_irq(tp->pdev->irq, dev);
-		if (tp->tg3_flags2 & TG3_FLG2_USING_MSI) {
-			pci_disable_msi(tp->pdev);
-			tp->tg3_flags2 &= ~TG3_FLG2_USING_MSI;
-		}
 		tg3_free_consistent(tp);
 		return err;
 	}

@@ -6121,10 +6113,6 @@
 			spin_lock_irq(&tp->lock);
 			spin_lock(&tp->tx_lock);
 
-			if (tp->tg3_flags2 & TG3_FLG2_USING_MSI) {
-				pci_disable_msi(tp->pdev);
-				tp->tg3_flags2 &= ~TG3_FLG2_USING_MSI;
-			}
 			tg3_halt(tp, RESET_KIND_SHUTDOWN, 1);
 			tg3_free_rings(tp);
 			tg3_free_consistent(tp);

@@ -6409,10 +6397,6 @@
 	spin_unlock_irq(&tp->lock);
 
 	free_irq(tp->pdev->irq, dev);
-	if (tp->tg3_flags2 & TG3_FLG2_USING_MSI) {
-		pci_disable_msi(tp->pdev);
-		tp->tg3_flags2 &= ~TG3_FLG2_USING_MSI;
-	}
 
 	memcpy(&tp->net_stats_prev, tg3_get_stats(tp->dev),
 	       sizeof(tp->net_stats_prev));

@@ -828,11 +828,13 @@
 	entries[1].entry = 1;
 	entries[2].entry = 2;
 
+	pci_disable_msi(mdev->pdev);
 	err = pci_enable_msix(mdev->pdev, entries, ARRAY_SIZE(entries));
 	if (err) {
 		if (err > 0)
 			mthca_info(mdev, "Only %d MSI-X vectors available, "
 				   "not using MSI-X\n", err);
+		pci_enable_msi(mdev->pdev);
 		return err;
 	}
 

@@ -1001,7 +1003,7 @@
 	if (msi_x && !mthca_enable_msi_x(mdev))
 		mdev->mthca_flags |= MTHCA_FLAG_MSI_X;
 	if (msi && !(mdev->mthca_flags & MTHCA_FLAG_MSI_X) &&
-	    !pci_enable_msi(pdev))
+	    pci_in_msi_mode(pdev))
 		mdev->mthca_flags |= MTHCA_FLAG_MSI;
 
 	sema_init(&mdev->cmd.hcr_sem, 1);

@@ -1076,8 +1078,6 @@
 err_free_dev:
 	if (mdev->mthca_flags & MTHCA_FLAG_MSI_X)
 		pci_disable_msix(pdev);
-	if (mdev->mthca_flags & MTHCA_FLAG_MSI)
-		pci_disable_msi(pdev);
 
 	ib_dealloc_device(&mdev->ib_dev);
 

@@ -1125,8 +1125,6 @@
 
 		if (mdev->mthca_flags & MTHCA_FLAG_MSI_X)
 			pci_disable_msix(pdev);
-		if (mdev->mthca_flags & MTHCA_FLAG_MSI)
-			pci_disable_msi(pdev);
 
 		ib_dealloc_device(&mdev->ib_dev);
 		mthca_release_regions(pdev, mdev->mthca_flags &

I would think the number of MSI and MSI-X capable devices is going to explode over the next five years. I'm not sure it's right to make all these device's drivers pay a complexity cost because some of the first attempted MSI implementations were buggy.

For now...but I'm bringing this up again in five years!! *sets egg timer*

-- Andy

Re-read the start of the thread :)

My suggestion...

short term: do nothing

long term: move drivers to a new pci_enable()/pci_disable() API which makes it easy for us to roll a lot of these singleton function calls, repeated over and over again in PCI drivers, into generic code.

Then just let evolution happen. That way, progress occurs, but no existing drivers are broken by a sudden pci_enable_device() behavior change.

Since we're in a "rolling stable series" that might be a better path to take. If evolution goes as expected, maybe there will no longer be any public users of pci_enable_msi()...

Jeff

As an aside...

Never forget that hardware vendors _always_ want their latest stuff to the default <whatever>. :)

In the grand scheme of things, I really don't see why we need to rush into anything, on the PCI MSI front. Support exists. Drivers work as-is today. Is it terribly urgent to default PCI MSI on in pci_enable_device()?

Things are still shaking out with regards to broken devices, and broken system chipsets.

Jeff

Exactly. Couldnt agree more.

I think we should have the right (default MSI) API by default. If we wait 5 years we end up with lots of mess in the drivers.

-Andi

Andi> I think we should have the right (default MSI) API by Andi> default. If we wait 5 years we end up with lots of mess in Andi> the drivers.

I agree that we should get the API correct as soon as we can. However I would argue that one property of the correct API would be that changing the default from MSI turned off to MSI turned on should not affect the API.

I like Jeff's suggestion of rolling more common code into pci_enable_device(). However I'm not sure if this fixes the MSI mess completely -- there may be some devices where the driver wants to test interrupt generation and disable MSI if it fails. However this may be enough of a special case that we can just add a flag to those particular drivers so that users can manually control the use of MSI.

- R.

I'd rather you dropped it or treated MSI/MSI-X more alike.

Just do it. :^)

IA64 supports MSI/MSI-X. Roland, didn't PPC as well?

I don't think it's *that* x86 specific. The base address of the Processor interrupt Block (IIRC 0xfee0000) is implemented the same on several arches AFAIK. Even on some parisc chipsets. I been constantly chasing other basic issues on parisc and just haven't had time to implement it in the past 3 years.

I also see one minor weakness in the assumption that CPU Vectors are global. Both IA64/PARISC can support per-CPU Vector tables. I thought some other arches could too but can't remember the details. Ie the same vector can be directed at different CPUs depending on which address the transaction targets. This vastly increases the number of unique vectors available on 2 or 4-way boxes. This is interesting if drivers start consuming multiple MSI-X vectors per card instance.

grant

x86-64 will eventually too, I definitely plan for it at some point. We need it for very big machines where 255 interrupt vectors are not enough. And as you say with MSI-X it becomes even more important.

-Andi

Andi> x86-64 will eventually too, I definitely plan for it at some Andi> point. We need it for very big machines where 255 interrupt Andi> vectors are not enough. And as you say with MSI-X it becomes Andi> even more important.

MSI-X already works fine on x86-64. For example, on the machine I'm sending this from, an Athlon64 system with Nforce 4 (and yes I am using IP-over-InfiniBand as the only network connection on my workstation):

    $ arch
    x86_64

    $ cat /proc/interrupts 
               CPU0       
      0: 1340159746    IO-APIC-edge  timer
      7:          2    IO-APIC-edge  parport0
      8:          0    IO-APIC-edge  rtc
      9:          0   IO-APIC-level  acpi
     15:   13333990    IO-APIC-edge  ide1
     50:          3       PCI-MSI-X  ib_mthca (async)
     58:     480844       PCI-MSI-X  ib_mthca (cmd)
    177:    7975072   IO-APIC-level  libata, NVidia CK804
    185:    2093654   IO-APIC-level  libata
    193:   14304257   IO-APIC-level  nvidia
    201:     769996   IO-APIC-level  ohci_hcd
    209:          2   IO-APIC-level  ehci_hcd
    217:          3   IO-APIC-level  ohci1394
    233:   37917879       PCI-MSI-X  ib_mthca (comp)
    NMI:     148617 
    LOC: 1340049308 
    ERR:          0
    MIS:          0

    $ lspci
    0000:00:00.0 Memory controller: nVidia Corporation CK804 Memory Controller (rev a3)
    0000:00:01.0 ISA bridge: nVidia Corporation: Unknown device 0050 (rev a3)
    0000:00:01.1 SMBus: nVidia Corporation CK804 SMBus (rev a2)
    0000:00:02.0 USB Controller: nVidia Corporation CK804 USB Controller (rev a2)
    0000:00:02.1 USB Controller: nVidia Corporation CK804 USB Controller (rev a3)
    0000:00:04.0 Multimedia audio controller: nVidia Corporation CK804 AC'97 Audio Controller (rev a2)
    0000:00:06.0 IDE interface: nVidia Corporation CK804 IDE (rev a2)
    0000:00:07.0 IDE interface: nVidia Corporation CK804 Serial ATA Controller (rev a3)
    0000:00:08.0 IDE interface: nVidia Corporation CK804 Serial ATA Controller (rev a3)
    0000:00:09.0 PCI bridge: nVidia Corporation CK804 PCI Bridge (rev a2)
    0000:00:0a.0 Bridge: nVidia Corporation CK804 Ethernet Controller (rev a3)
    0000:00:0b.0 PCI bridge: nVidia Corporation CK804 PCIE Bridge (rev a3)
    0000:00:0c.0 PCI bridge: nVidia Corporation CK804 PCIE Bridge (rev a3)
    0000:00:0d.0 PCI bridge: nVidia Corporation CK804 PCIE Bridge (rev a3)
    0000:00:0e.0 PCI bridge: nVidia Corporation CK804 PCIE Bridge (rev a3)
    0000:00:18.0 Host bridge: Advanced Micro Devices [AMD] K8 [Athlon64/Opteron] HyperTransport Technology Configuration
    0000:00:18.1 Host bridge: Advanced Micro Devices [AMD] K8 [Athlon64/Opteron] Address Map
    0000:00:18.2 Host bridge: Advanced Micro Devices [AMD] K8 [Athlon64/Opteron] DRAM Controller
    0000:00:18.3 Host bridge: Advanced Micro Devices [AMD] K8 [Athlon64/Opteron] Miscellaneous Control
    0000:01:00.0 VGA compatible controller: nVidia Corporation: Unknown device 0140 (rev a2)
    0000:03:00.0 InfiniBand: Mellanox Technologies MT25208 InfiniHost III Ex (rev a0)
    0000:05:0b.0 FireWire (IEEE 1394): Texas Instruments TSB43AB22/A IEEE-1394a-2000 Controller (PHY/Link)
    0000:05:0c.0 Ethernet controller: Marvell Technology Group Ltd. Gigabit Ethernet Controller (rev 13)

    $ cat /proc/cpuinfo 
    processor	: 0
    vendor_id	: AuthenticAMD
    cpu family	: 15
    model		: 31
    model name	: AMD Athlon(tm) 64 Processor 3500+
    stepping	: 0
    cpu MHz		: 1005.169
    cache size	: 512 KB
    fpu		: yes
    fpu_exception	: yes
    cpuid level	: 1
    wp		: yes
    flags		: fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat pse36 clflush mmx fxsr sse sse2 pni syscall nx mmxext fxsr_opt lm 3dnowext 3dnow lahf_lm
    bogomips	: 1988.42
    TLB size	: 1088 4K pages
    clflush size	: 64
    cache_alignment	: 64
    address sizes	: 40 bits physical, 48 bits virtual
    power management: ts fid vid ttp

Yes, but only as long as you dont have too many devices. The problem is that there are only 255 interrupt vectors right now, and that is not enough for bigger systems with many devices.

A single CPU box does not even appear on the radar here.

-Andi

One thing to keep in mind is that since now we have support for CPU hotplug we need to factor in cases when cpu is removed, the per-cpu vectors would require migrating to a new cpu far interrupt target. Which would possibly require vector-sharing support as well in case the vector is used in all other cpus.

Possibly irq balancer might need to be revisited as well, and potentially might trigger some sharing needs.

A combination of
- Not allocating IRQs to pins not used (Which Natalie from Unisys submitted)
http://marc.theaimsgroup.com/?l=linux-kernel&m=111656957923038&w=2
- per-cpu vector tables (long back i remember seeing some post from sgi on the topic, possibly under intr domains etc.. not too sure)
- vector sharing

Cheers,
Ashok Raj
- Open Source Technology Center

I did something for i386 which setup per node vector tables, i resurrected it for newer systems (ES7000) but haven't fixed MSI support yet. But that may not be what you're referring to ;)

This might be simpler if we did IRQ handling domains and setup a group of processors with the same vector tables. That way we just migrate onto one of the other cpus in the IRQ handling domain. This should also leave plenty of room for many devices per IRQ handling domain (my reference is a 4 processor per IRQ domain).

Thanks,
Zwane

Yes, it would need require vector migration. I suppose the way to do this would be to not offline a CPU as long as there are devices that have interrupts refering to a CPU. And perhaps have some /sys file that prevents allocating new vectors to a specific CPU.

User space could then do:

Set sys file to prevent new vectors allocated to cpu FOO Read /proc/interrupts and unload all drivers who have vectors on CPU foo. Reload drivers.
Offline CPU.

BTW I suppose in many machines the problem would not occur because they do hotplug not on inidividual CPUs, but nodes which consist of CPUs,PCI busses etc. On those you could fully avoid it by just making sure the devices on the PCI busses of a node only allocate interrupts on the local CPUs. Then to off line you would need to hot unplug the devices before offlining the CPUs anyways.

goal anyways to avoid cross node traffic.

IRQ balancer is in user space on x86-64.

That is already queued, but doesnt help in the general case.

That is the plan.

That doesnt help in the general case again I think.

e.g. to fully cover offlining of all CPus you would need to share all vectors over all CPUs of all devices. But that totally defeats the original goal to get more than 255 vectors.

-Andi

Sounds way too complicated approach!

In current scheme we do in fixup_irqs() if a irq is bound to a cpu we migrate it to another online cpu.

disable(), reprogram new cpu destination (i.e new vector), and then re-enable

Even for MSI we do this via config cycle access, if set_msi_affinity() works, then the re-program should work as well.

If you want to unload drivers, then apps would need to stop... this sounds more like a M$ solution :-(, we are far better off to reboot in that case?

But if your entire Node needs to be offlined because a failure is happening and you need to replace that node, you could offline all the cpu's which would require irqs to go off node anyway until the node is replaced.

If its just a failing cpu, and you want to keep that off execution path, then with intr domains, and the 4-cpus in that node forming a domain, fixup_irqs() could pick another cpu within the same domain for that irq.

more like what zwane had proposed, but would require reprogramming msi's at devices anyway.

I could be wrong, but the idea is if you end up all vectors are used in any other domain, then we could use vector sharing. Agreed sharing has performance issues, since we would end up sharing IRQS's, and chained intr calls, but it might be the best you can do for RAS reasons. If downtime is not preferred. (Longer term, maybe we could have some policy from user space that could honor such requests and be able to customize..)

Cheers,
Ashok Raj
- Open Source Technology Center

Greg> I've also seen some complaints that it is very arch specific Greg> (x86 based). But as no other arches seem to want to support Greg> MSI, I don't really see any need to split it up. Any Greg> comments about this?

Grant> IA64 supports MSI/MSI-X. Roland, didn't PPC as well?

Grant> I don't think it's *that* x86 specific. The base address Grant> of the Processor interrupt Block (IIRC 0xfee0000) is Grant> implemented the same on several arches AFAIK. Even on some Grant> parisc chipsets. I been constantly chasing other basic Grant> issues on parisc and just haven't had time to implement it Grant> in the past 3 years.

Some PPC does support MSI. For example the PCI-X host bridge inside the IBM (now AMCC) PPC 440GP supports MSI. I don't know if any non-embedded PPC or PPC64 supports MSI though -- for example do any IBM POWER-based systems support MSI??

The current code isn't exactly x86-specific. It's more Intel APIC specific, which means that chipsets that for some reason or another support the same address/message format will work. So ia64 works as well. Following from that, HP parisc boxes that use the same chipset as HP ia64 boxes have a chance at working.

But the PPC 440GP uses a different address and a different message format. So if anyone cared about supporting that, the current code would have to be made more generic.

- R.

The problem is then that we have to go through all drivers and add the ugly logic there. Isnt it better to do it by default?

I think it would be better to have a clean split.

-Andi

No, the logic to enable MSI in a driver today is simple, and straight-forward. A single call to pci_enable_msi(). If instead the driver wants to enable MSI-X, they call pci_enable_msix(). Contrast that with the logic of:

	- if MSI is enabled, disable it for some devices.
	- if driver wants to enable MSI-X, do:
		pci_disable_msi();
		e = pci_enable_msix();	// which I think some people
					// said will always fail anyway
					// due to MSI being enabled
					// already.
		if (e)
			pci_enable_msi();

So, we do that, MSI-X is now more complex and harder to enable (and that's the future for PCI devices from what people are saying.)

Anyway, I'm just repeating myself now...

thanks,

greg k-h

Agreed.

Agreed.

Drop it. Yes.

It's up to you.

Luben

OT, what I'd like see changed with current MSI/MSI-X code is the masking/unmasking of vectors currently performed on each interrupt.

Note that in MSI some devices dont support per-vector masking, while in MSI-X the masking is performed by means of PCI write with no flush, which might not yet reach the device by the time the handler is called.

IMHO, its better to have helper functions for drivers to mask/unmask vectors if/when they need to, removing some overhead for drivers that dont need this masking, and giving drivers the ability to combine the masking write with read to actually mask interrupts if they do need it.

Does this make sense to anyone?

MST