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

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

[Greg KH]

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.

Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>


---
 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(-)

--- gregkh-2.6.orig/drivers/pci/msi.c	2005-06-06 16:16:25.000000000 -0700
+++ gregkh-2.6/drivers/pci/msi.c	2005-06-06 17:01:35.000000000 -0700
@@ -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);
--- gregkh-2.6.orig/include/linux/pci.h	2005-06-06 16:16:25.000000000 -0700
+++ gregkh-2.6/include/linux/pci.h	2005-06-06 16:29:59.000000000 -0700
@@ -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);
--- gregkh-2.6.orig/drivers/pci/pci.c	2005-06-06 16:16:30.000000000 -0700
+++ gregkh-2.6/drivers/pci/pci.c	2005-06-06 16:20:06.000000000 -0700
@@ -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;
--- gregkh-2.6.orig/drivers/net/bnx2.c	2005-06-06 12:08:46.000000000 -0700
+++ gregkh-2.6/drivers/net/bnx2.c	2005-06-06 16:30:30.000000000 -0700
@@ -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);
--- gregkh-2.6.orig/drivers/pci/pci.h	2005-06-06 12:08:52.000000000 -0700
+++ gregkh-2.6/drivers/pci/pci.h	2005-06-06 16:29:47.000000000 -0700
@@ -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
--- gregkh-2.6.orig/drivers/pci/hotplug/shpchp_hpc.c	2005-03-01 23:37:52.000000000 -0800
+++ gregkh-2.6/drivers/pci/hotplug/shpchp_hpc.c	2005-06-06 16:33:25.000000000 -0700
@@ -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);
--- gregkh-2.6.orig/drivers/pci/pcie/portdrv_core.c	2005-06-06 12:23:18.000000000 -0700
+++ gregkh-2.6/drivers/pci/pcie/portdrv_core.c	2005-06-06 16:32:24.000000000 -0700
@@ -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)
--- gregkh-2.6.orig/drivers/net/e1000/e1000_main.c	2005-06-06 12:08:47.000000000 -0700
+++ gregkh-2.6/drivers/net/e1000/e1000_main.c	2005-06-06 16:52:02.000000000 -0700
@@ -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);
--- gregkh-2.6.orig/drivers/net/ixgb/ixgb_main.c	2005-06-06 12:08:48.000000000 -0700
+++ gregkh-2.6/drivers/net/ixgb/ixgb_main.c	2005-06-06 16:54:49.000000000 -0700
@@ -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
--- gregkh-2.6.orig/drivers/net/tg3.c	2005-06-06 12:08:51.000000000 -0700
+++ gregkh-2.6/drivers/net/tg3.c	2005-06-06 16:57:34.000000000 -0700
@@ -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));
--- gregkh-2.6.orig/drivers/infiniband/hw/mthca/mthca_main.c	2005-06-06 12:08:42.000000000 -0700
+++ gregkh-2.6/drivers/infiniband/hw/mthca/mthca_main.c	2005-06-06 17:01:37.000000000 -0700
@@ -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 &
--- gregkh-2.6.orig/drivers/pci/hotplug/pciehp_hpc.c	2005-06-06 12:08:52.000000000 -0700
+++ gregkh-2.6/drivers/pci/hotplug/pciehp_hpc.c	2005-06-06 17:02:40.000000000 -0700
@@ -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) 

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

[Andrew Vasquez]

On Mon, 06 Jun 2005, Greg KH wrote:

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

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

[Greg KH]

On Mon, Jun 06, 2005 at 06:09:11PM -0700, Andrew Vasquez wrote:
> On Mon, 06 Jun 2005, Greg KH wrote:
> 
> > 
> > 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 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).

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

> * 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?

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

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

[Arjan van de Ven]

> > * 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).
> 
> Yes, a very good point.  I guess I should keep the pci_enable_msi() and
> pci_disable_msi() functions exported for this reason.
> 

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

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

[Grant Grundler]

On Tue, Jun 07, 2005 at 09:31:39AM +0200, Arjan van de Ven wrote:
> 
> > > * 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).
> > 
> > Yes, a very good point.  I guess I should keep the pci_enable_msi() and
> > pci_disable_msi() functions exported for this reason.
> > 
> 
> 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

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

[Greg KH]

On Tue, Jun 07, 2005 at 09:31:39AM +0200, Arjan van de Ven wrote:
> 
> > > * 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).
> > 
> > Yes, a very good point.  I guess I should keep the pci_enable_msi() and
> > pci_disable_msi() functions exported for this reason.
> > 
> 
> well... only pci_disable_msi() is needed for this ;)

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

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

[Arjan van de Ven]

On Tue, 2005-06-07 at 09:10 -0700, Greg KH wrote:
> On Tue, Jun 07, 2005 at 09:31:39AM +0200, Arjan van de Ven wrote:
> > 
> > > > * 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).
> > > 
> > > Yes, a very good point.  I guess I should keep the pci_enable_msi() and
> > > pci_disable_msi() functions exported for this reason.
> > > 
> > 
> > well... only pci_disable_msi() is needed for this ;)
> 
> 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...)

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

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

[Greg KH]

On Tue, Jun 07, 2005 at 10:41:12PM +0200, Arjan van de Ven wrote:
> On Tue, 2005-06-07 at 09:10 -0700, Greg KH wrote:
> > On Tue, Jun 07, 2005 at 09:31:39AM +0200, Arjan van de Ven wrote:
> > > 
> > > > > * 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).
> > > > 
> > > > Yes, a very good point.  I guess I should keep the pci_enable_msi() and
> > > > pci_disable_msi() functions exported for this reason.
> > > > 
> > > 
> > > well... only pci_disable_msi() is needed for this ;)
> > 
> > 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...)
> 
> 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

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

[Roland Dreier]

    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.

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

[Grant Grundler]

On Tue, Jun 07, 2005 at 03:43:53PM -0700, Roland Dreier wrote:
> I'm sure other MSI-X capable devices can slice and dice things differently.

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

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

[Andi Kleen]

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

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

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

[Andi Kleen]

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

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

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

[Grant Grundler]

On Wed, Jun 08, 2005 at 03:37:32PM +0200, Andi Kleen wrote:
> > 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...)
> 
> 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...

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

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

[Stefan Smietanowski]

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

Hi Greg.

>>>>* 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).
>>>
>>>Yes, a very good point.  I guess I should keep the pci_enable_msi() and
>>>pci_disable_msi() functions exported for this reason.
>>>
>>
>>well... only pci_disable_msi() is needed for this ;)
> 
> 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...

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

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

[Greg KH]

On Thu, Jun 09, 2005 at 06:54:31AM +0200, Stefan Smietanowski wrote:
> > 
> > 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...
> 
> 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.

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

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

[Grant Grundler]

On Mon, Jun 06, 2005 at 05:20:45PM -0700, Greg KH wrote:
> -EXPORT_SYMBOL(pci_enable_msi);
> -EXPORT_SYMBOL(pci_disable_msi);
> +//EXPORT_SYMBOL(pci_enable_msi);
> +//EXPORT_SYMBOL(pci_disable_msi);

You do plan on deleting these lines, right? (Just a reminder)

> --- gregkh-2.6.orig/drivers/pci/pci.c	2005-06-06 16:16:30.000000000 -0700
> +++ gregkh-2.6/drivers/pci/pci.c	2005-06-06 16:20:06.000000000 -0700
> @@ -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;
>  }

I'm wondering if it would be better/possible to call
enable_msi before calling fixup_device.
But I couldn't find any references to DECLARE_PCI_FIXUP_ENABLE
to see if it would matter.


> @@ -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;

reminder: you probably need to check that msi is still enabled
here or in pci_disable_msi(). I'm thinking of the case where
the driver switched to MSI-X.


> +#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

wouldn't it make more sense to invert this to "ifdef" (vs ifndef)?

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

Why disable msix here but not msi?
This doesn't seem like a good design choice.
I'm suspicious more occurrances will need special handling of MSIX.

Will assign_interrupt_mode() in drivers/pci/pcie/portdrv_core.c
need more changes to deal with the switch to/from MSI/MSIX?


> +++ gregkh-2.6/drivers/net/tg3.c	2005-06-06 16:57:34.000000000 -0700
> @@ -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;

This chunk of tg3 code will need more than just deleting
the call to pci_disable_msi(). The printk will be wrong
and it's changing the semantics of what TG3_FLG2_USING_MSI
means.

> @@ -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)) {

Again - the "Not Using MSI" msg here needs more help to be correct.


hth,
grant

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

[Greg KH]

On Tue, Jun 07, 2005 at 09:53:24AM -0600, Grant Grundler wrote:
> On Mon, Jun 06, 2005 at 05:20:45PM -0700, Greg KH wrote:
> > -EXPORT_SYMBOL(pci_enable_msi);
> > -EXPORT_SYMBOL(pci_disable_msi);
> > +//EXPORT_SYMBOL(pci_enable_msi);
> > +//EXPORT_SYMBOL(pci_disable_msi);
> 
> You do plan on deleting these lines, right? (Just a reminder)

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

[RFC PATCH] PCI: remove access to pci_[enable|disable]_msi() for drivers - take 2

[Greg KH]

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.

Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>


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

--- gregkh-2.6.orig/drivers/pci/msi.c	2005-06-06 16:16:25.000000000 -0700
+++ gregkh-2.6/drivers/pci/msi.c	2005-06-06 22:16:14.000000000 -0700
@@ -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
  *
--- gregkh-2.6.orig/include/linux/pci.h	2005-06-06 16:16:25.000000000 -0700
+++ gregkh-2.6/include/linux/pci.h	2005-06-06 22:17:03.000000000 -0700
@@ -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);
--- gregkh-2.6.orig/drivers/pci/pci.c	2005-06-06 16:16:30.000000000 -0700
+++ gregkh-2.6/drivers/pci/pci.c	2005-06-06 16:20:06.000000000 -0700
@@ -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;
--- gregkh-2.6.orig/drivers/net/bnx2.c	2005-06-06 12:08:46.000000000 -0700
+++ gregkh-2.6/drivers/net/bnx2.c	2005-06-06 22:49:38.000000000 -0700
@@ -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);
 	}
--- gregkh-2.6.orig/drivers/net/e1000/e1000_main.c	2005-06-06 12:08:47.000000000 -0700
+++ gregkh-2.6/drivers/net/e1000/e1000_main.c	2005-06-06 22:56:45.000000000 -0700
@@ -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);
--- gregkh-2.6.orig/drivers/net/ixgb/ixgb_main.c	2005-06-06 12:08:48.000000000 -0700
+++ gregkh-2.6/drivers/net/ixgb/ixgb_main.c	2005-06-06 23:02:18.000000000 -0700
@@ -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
--- gregkh-2.6.orig/drivers/net/tg3.c	2005-06-06 12:08:51.000000000 -0700
+++ gregkh-2.6/drivers/net/tg3.c	2005-06-06 22:53:49.000000000 -0700
@@ -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));
--- gregkh-2.6.orig/drivers/infiniband/hw/mthca/mthca_main.c	2005-06-06 12:08:42.000000000 -0700
+++ gregkh-2.6/drivers/infiniband/hw/mthca/mthca_main.c	2005-06-06 23:04:25.000000000 -0700
@@ -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 &

Re: [RFC PATCH] PCI: remove access to pci_[enable|disable]_msi() for drivers - take 2

[Jeff Garzik]

Greg KH wrote:
> @@ -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;
>  	}


If the driver has to _undo_ something that it did not do, that's pretty 
lame.  Non-orthogonal.

Also, it looks like all the PCI MSI drivers need touching for this 
scheme -- which defeats the original intention.  At this rate, the best 
API is the one we've already got.

	Jeff

Re: [RFC PATCH] PCI: remove access to pci_[enable|disable]_msi() for drivers - take 2

[Andrew Grover]

On 6/7/05, Jeff Garzik <jgarzik@pobox.com> wrote:
> If the driver has to _undo_ something that it did not do, that's pretty
> lame.  Non-orthogonal.

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.

> Also, it looks like all the PCI MSI drivers need touching for this
> scheme -- which defeats the original intention.  At this rate, the best
> API is the one we've already got.

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

-- Andy

Re: [RFC PATCH] PCI: remove access to pci_[enable|disable]_msi() for drivers - take 2

[Jeff Garzik]

Andrew Grover wrote:
> On 6/7/05, Jeff Garzik <jgarzik@pobox.com> wrote:
>>Also, it looks like all the PCI MSI drivers need touching for this
>>scheme -- which defeats the original intention.  At this rate, the best
>>API is the one we've already got.
> 
> 
> For now...but I'm bringing this up again in five years!! *sets egg timer*


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

Re: [RFC PATCH] PCI: remove access to pci_[enable|disable]_msi() for drivers - take 2

[Jeff Garzik]

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

Re: [RFC PATCH] PCI: remove access to pci_[enable|disable]_msi() for drivers - take 2

[Andi Kleen]

On Tue, Jun 07, 2005 at 10:55:27PM -0700, Andrew Grover wrote:
> On 6/7/05, Jeff Garzik <jgarzik@pobox.com> wrote:
> > If the driver has to _undo_ something that it did not do, that's pretty
> > lame.  Non-orthogonal.
> 
> 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.

Exactly. Couldnt agree more.
> 
> > Also, it looks like all the PCI MSI drivers need touching for this
> > scheme -- which defeats the original intention.  At this rate, the best
> > API is the one we've already got.
> 
> For now...but I'm bringing this up again in five years!! *sets egg timer*

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

Re: [RFC PATCH] PCI: remove access to pci_[enable|disable]_msi() for drivers - take 2

[Roland Dreier]

    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.

Re: [RFC PATCH] PCI: remove access to pci_[enable|disable]_msi() for drivers - take 2

[Grant Grundler]

On Tue, Jun 07, 2005 at 01:21:29PM -0700, Greg KH wrote:
> 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?

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

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

Just do it. :^)

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

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

Re: [RFC PATCH] PCI: remove access to pci_[enable|disable]_msi() for drivers - take 2

[Andi Kleen]

> I also see one minor weakness in the assumption that CPU Vectors
> are global. Both IA64/PARISC can support per-CPU Vector tables.

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

Re: [RFC PATCH] PCI: remove access to pci_[enable|disable]_msi() for drivers - take 2

[Roland Dreier]

    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

Re: [RFC PATCH] PCI: remove access to pci_[enable|disable]_msi() for drivers - take 2

[Andi Kleen]

On Wed, Jun 08, 2005 at 08:52:26AM -0700, Roland Dreier wrote:
>     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

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.

> 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):

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

-Andi

Re: [RFC PATCH] PCI: remove access to pci_[enable|disable]_msi() for drivers - take 2

[Ashok Raj]

On Wed, Jun 08, 2005 at 06:32:26AM -0700, Andi Kleen wrote:
> 
>    > I also see one minor weakness in the assumption that CPU Vectors
>    > are global. Both IA64/PARISC can support per-CPU Vector tables.

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

> 
>    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
>    -
>    To   unsubscribe   from   this   list:   send  the  line  "unsubscribe
>    linux-kernel" in
>    the body of a message to majordomo@vger.kernel.org
>    More majordomo info at  [1]http://vger.kernel.org/majordomo-info.html
>    Please read the FAQ at  [2]http://www.tux.org/lkml/
> 
> References
> 
>    1. http://vger.kernel.org/majordomo-info.html
>    2. http://www.tux.org/lkml/

-- 
Cheers,
Ashok Raj
- Open Source Technology Center

Re: [RFC PATCH] PCI: remove access to pci_[enable|disable]_msi() for drivers - take 2

[Zwane Mwaikambo]

On Wed, 8 Jun 2005, Ashok Raj wrote:

> On Wed, Jun 08, 2005 at 06:32:26AM -0700, Andi Kleen wrote:
> > 
> >    > I also see one minor weakness in the assumption that CPU Vectors
> >    > are global. Both IA64/PARISC can support per-CPU Vector tables.
> 
> 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)

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

>  - vector sharing

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

Re: [RFC PATCH] PCI: remove access to pci_[enable|disable]_msi() for drivers - take 2

[Andi Kleen]

On Wed, Jun 08, 2005 at 09:09:44AM -0700, Ashok Raj wrote:
> On Wed, Jun 08, 2005 at 06:32:26AM -0700, Andi Kleen wrote:
> > 
> >    > I also see one minor weakness in the assumption that CPU Vectors
> >    > are global. Both IA64/PARISC can support per-CPU Vector tables.
> 
> 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.


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.
>From a NUMA optimization perspective that is a worthy
goal anyways to avoid cross node traffic.
	
	
> 
> Possibly irq balancer might need to be revisited as well, and potentially
> might trigger some sharing needs.

IRQ balancer is in user space on x86-64.

> 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

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

>  - per-cpu vector tables (long back i remember seeing some post from sgi
>    on the topic, possibly under intr domains etc.. not too sure)

That is the plan.

>  - vector sharing

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

Re: [RFC PATCH] PCI: remove access to pci_[enable|disable]_msi() for drivers - take 2

[Ashok Raj]

On Thu, Jun 09, 2005 at 04:11:34PM +0200, Andi Kleen wrote:
> On Wed, Jun 08, 2005 at 09:09:44AM -0700, Ashok Raj wrote:
> > On Wed, Jun 08, 2005 at 06:32:26AM -0700, Andi Kleen wrote:
> > > 
> > >    > I also see one minor weakness in the assumption that CPU Vectors
> > >    > are global. Both IA64/PARISC can support per-CPU Vector tables.
> > 
> > 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.
> 
> 
> 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.

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?
> 
> 
> 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.
> From a NUMA optimization perspective that is a worthy
> goal anyways to avoid cross node traffic.
> 	
>

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. 

> > 
> >    on the topic, possibly under intr domains etc.. not too sure)
> 
> That is the plan.
> 
> >  - vector sharing
> 
> 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.

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

Re: [RFC PATCH] PCI: remove access to pci_[enable|disable]_msi() for drivers - take 2

[Roland Dreier]

    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.

Re: [RFC PATCH] PCI: remove access to pci_[enable|disable]_msi() for drivers - take 2

[Andi Kleen]

On Tue, Jun 07, 2005 at 01:21:29PM -0700, Greg KH wrote:
> 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.

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?

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

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

-Andi

Re: [RFC PATCH] PCI: remove access to pci_[enable|disable]_msi() for drivers - take 2

[Greg KH]

On Wed, Jun 08, 2005 at 03:31:09PM +0200, Andi Kleen wrote:
> On Tue, Jun 07, 2005 at 01:21:29PM -0700, Greg KH wrote:
> > 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.
> 
> 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?

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

Re: [RFC PATCH] PCI: remove access to pci_[enable|disable]_msi() for drivers - take 2

[Luben Tuikov]

On 06/07/05 16:21, Greg KH wrote:
> 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

Agreed.

> 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

Agreed.

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

Drop it.  Yes.

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

It's up to you.

	Luben

Re: [RFC PATCH] PCI: remove access to pci_[enable|disable]_msi() for drivers - take 2

[Michael S. Tsirkin]

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

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

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

[Nguyen, Tom L]

Monday, June 06, 2005 10:16 PM Greg KH wrote:
> On Mon, Jun 06, 2005 at 06:09:11PM -0700, Andrew Vasquez wrote:
> > * 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).

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

That is one of the reason we like to have a driver to call explicitly
either pci_enable_msi() or pci_enable_msix() since its device may
support either or both MSI capability structure and MSI-X capability
structure. Once either MSI or MSI-X is enabled, the existing code does
not allow a driver to switch back and forth between MSI and MSI-X.

In addition, if you plan to enable MSI by default, then new function
pci_in_msi_mode is redundant because pci_enable_msi() always returns
zero if MSI was already enabled on a device. In this case, a driver must
use MSI, not MSI-X. As a result, I still prefer to have a driver to
explicitly call pci_enable_msi().

Thanks,
Long