Re: [PATCH net-next-2.6 1/2] be2net: Adding an option to use INTx instead of MSI-X

Previous thread: [PATCH 2/2] be2net: Fix CSO for UDP packets by Somnath Kotur on Monday, October 25, 2010 - 4:11 am. (2 messages)

Next thread: [PATCH net-next-2.6 2/2] be2net: Schedule/Destroy worker thread in probe()/remove() rather than open()/close() by Somnath Kotur on Monday, October 25, 2010 - 4:13 am. (1 message)
From: Somnath Kotur
Date: Monday, October 25, 2010 - 4:12 am

By default, be2net uses MSIx wherever possible.
Adding a module parameter to use INTx for users who do not want to use MSIx.

Signed-off-by: Somnath Kotur <somnath.kotur@emulex.com>
---
 drivers/net/benet/be_main.c |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/drivers/net/benet/be_main.c b/drivers/net/benet/be_main.c
index 45b1f66..1262292 100644
--- a/drivers/net/benet/be_main.c
+++ b/drivers/net/benet/be_main.c
@@ -27,10 +27,13 @@ MODULE_LICENSE("GPL");
 
 static unsigned int rx_frag_size = 2048;
 static unsigned int num_vfs;
+static unsigned int msix = 1;
 module_param(rx_frag_size, uint, S_IRUGO);
 module_param(num_vfs, uint, S_IRUGO);
+module_param(msix, uint, S_IRUGO);
 MODULE_PARM_DESC(rx_frag_size, "Size of a fragment that holds rcvd data.");
 MODULE_PARM_DESC(num_vfs, "Number of PCI VFs to initialize");
+MODULE_PARM_DESC(msix, "Enable/Disable the MSIx (MSIx enabled by default)");
 
 static bool multi_rxq = true;
 module_param(multi_rxq, bool, S_IRUGO | S_IWUSR);
@@ -2856,7 +2859,8 @@ static int __devinit be_probe(struct pci_dev *pdev,
 	if (status)
 		goto stats_clean;
 
-	be_msix_enable(adapter);
+	if (msix)
+		be_msix_enable(adapter);
 
 	INIT_DELAYED_WORK(&adapter->work, be_worker);
 
@@ -3082,6 +3086,12 @@ static int __init be_init_module(void)
 		num_vfs = 32;
 	}
 
+	if (!msix && num_vfs > 0) {
+		printk(KERN_WARNING DRV_NAME
+			" : MSIx required for num_vfs > 0. Ignoring msix=0\n");
+		msix = 1;
+	}
+
 	return pci_register_driver(&be_driver);
 }
 module_init(be_init_module);
-- 
1.5.6.1

--

From: David Miller
Date: Monday, October 25, 2010 - 12:09 pm

From: Somnath Kotur <somnath.kotur@emulex.com>

Either add a new ethtool flag, or use the PCI subsystem facilities
for tweaking things to implement this.

Do not use a module option, otherwise every other networking driver
author will get the same "cool" idea, give the module option
different names, and the resulting user experience is terrible.
--

From: Ben Hutchings
Date: Monday, October 25, 2010 - 3:38 pm

This has already happened, sadly.  So far as I can see it's mostly done
to allow users to work around systems with broken MSIs; I'm not aware of
any other reason to prefer legacy interrupts.  However, the PCI subsystem
already implements a blacklist and a kernel parameter for disabling MSIs
on these systems.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
--

From: David Miller
Date: Monday, October 25, 2010 - 4:25 pm

From: Ben Hutchings <bhutchings@solarflare.com>

The PCI subsystem bits I'm totally fine with.

But in the drivers themselves, that's what I don't want.
--

From: Somnath.Kotur
Date: Monday, October 25, 2010 - 10:24 pm

Dave,
         Could you pls ignore this patch and apply the next patch in the series ?
 ( PATCH 2/2]  be2net: Schedule/Destroy worker thread in probe()/remove() rather than open()/close()

Thanks
Som
________________________________________
From: David Miller [davem@davemloft.net]
Sent: Tuesday, October 26, 2010 4:55 AM
To: bhutchings@solarflare.com
Cc: Kotur, Somnath; netdev@vger.kernel.org; linux-pci@vger.kernel.org
Subject: Re: [PATCH net-next-2.6 1/2] be2net: Adding an option to use INTx instead of MSI-X

From: Ben Hutchings <bhutchings@solarflare.com>

The PCI subsystem bits I'm totally fine with.

But in the drivers themselves, that's what I don't want.
--

From: Michael Ellerman
Date: Monday, October 25, 2010 - 11:52 pm

That horse has really really bolted, it's gawn.

I count 26 drivers with "disable MSI/X" parameters. Some even have more
than one.

11 of them are network drivers, 9 scsi, 3 ata.

I agree it's a mess for users, but it's probably preferable to a
non-working driver.

Ethtool would be nice, but only for network drivers. Is there a generic
solution, quirks are obviously not keeping people happy.

cheers



MSI:

Param "msi", 10 users:
  drivers/ata/sata_mv.c:MODULE_PARM_DESC(msi, "Enable use of PCI MSI (0=off, 1=on)");
  drivers/ata/sata_nv.c:MODULE_PARM_DESC(msi, "Enable use of MSI (Default: false)");
  drivers/ata/sata_sil24.c:MODULE_PARM_DESC(msi, "Enable MSI (Default: false)");
  drivers/net/cxgb3/cxgb3_main.c:MODULE_PARM_DESC(msi, "whether to use MSI or MSI-X");
  drivers/net/cxgb4/cxgb4_main.c:MODULE_PARM_DESC(msi, "whether to use INTx (0), MSI (1) or MSI-X (2)");
  drivers/net/cxgb4vf/cxgb4vf_main.c:MODULE_PARM_DESC(msi, "whether to use MSI-X or MSI");
  drivers/net/forcedeth.c:MODULE_PARM_DESC(msi, "MSI interrupts are enabled by setting to 1 and disabled by setting to 0.");
  drivers/net/qla3xxx.c:MODULE_PARM_DESC(msi, "Turn on Message Signaled Interrupts.");
  drivers/scsi/aacraid/aachba.c:MODULE_PARM_DESC(msi, "IRQ handling."
  drivers/scsi/stex.c:MODULE_PARM_DESC(msi, "Enable Message Signaled Interrupts(0=off, 1=on)");

Param "use_msi", 4 users:
  drivers/net/qlcnic/qlcnic_main.c:MODULE_PARM_DESC(use_msi, "MSI interrupt (0=disabled, 1=enabled");
  drivers/scsi/3w-9xxx.c:MODULE_PARM_DESC(use_msi, "Use Message Signaled Interrupts.  Default: 0");
  drivers/scsi/3w-sas.c:MODULE_PARM_DESC(use_msi, "Use Message Signaled Interrupts. Default: 0");
  drivers/scsi/aic94xx/aic94xx_init.c:MODULE_PARM_DESC(use_msi, "\n"

Param "disable_msi", 4 users:
  drivers/scsi/vmw_pvscsi.c:MODULE_PARM_DESC(disable_msi, "Disable MSI use in driver - (default=0)");
  drivers/net/bnx2.c:MODULE_PARM_DESC(disable_msi, "Disable Message Signaled Interrupt (MSI)");
  ...
From: Ben Hutchings
Date: Tuesday, October 26, 2010 - 6:32 am

Since this is (normally) a property of the system, pci=nomsi is the
generic solution.

[...]

  drivers/net/sfc/efx.c:MODULE_PARM_DESC(interrupt_mode,
  drivers/net/sfc/efx.c-                 "Interrupt mode (0=>MSIX 1=>MSI 2=>legacy)");

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
--

From: Michael Ellerman
Date: Tuesday, October 26, 2010 - 4:20 pm

Sort of, it's a big hammer. Did all these driver writers not know about
pci=nomsi or did they prefer to add a parameter to their driver for some

Well spotted.

cheers


From: David Miller
Date: Wednesday, October 27, 2010 - 8:46 am

From: Michael Ellerman <michael@ellerman.id.au>

Every time I've actually done the work to try and track down the
true issue, it always turned out to be a PCI chipset problem rather
than a device specific issue.
--

From: Grant Grundler
Date: Wednesday, October 27, 2010 - 10:37 pm

I agree with your generalization. I can think of only one exception:
ISTR pre-5705 tg3 chips would send both MSI and assert IRQ line at the same time.

My guess is driver writers just want knob to "work around" any issues
*their* driver might see with chipset. Disabling MSI for all drivers
doesn't leave opportunity for experimenting with individual drivers.

hth,
--

From: David Miller
Date: Wednesday, October 27, 2010 - 8:45 am

From: Michael Ellerman <michael@ellerman.id.au>

Stupid inappropriate things being in the tree doesn't mean I need
to accept more of them.
--

From: Matthew Wilcox
Date: Saturday, October 30, 2010 - 4:21 pm

That's 26 patches someone needs to write, then.  You can put my Acked-by

What more drivers need is an automatic detection of a non-working
interrupt situation, great big warning messages, and fallback to an
alternate interrupt mechanism.  Doing it for one driver, then generalising
as much of it into the core as possible would be nice.

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."
--

From: Michael Ellerman
Date: Wednesday, November 3, 2010 - 5:45 am

Bah, come on it's hardly the most horrendous sin committed by driver
writers. And removing them risks breaking someone's system, even if they

More detection would be good.

I don't see much potential for generalising it though. Looking at e1000e
and tg3 there is really not much in common except the very basic idea.

cheers


From: David Miller
Date: Wednesday, November 3, 2010 - 8:28 am

From: Michael Ellerman <michael@ellerman.id.au>

Yes but it's pretty high on the list because it deteriorates the
user experience.

It's a real selfish move to try and "fix" things in this way and
we've established that this isn't acceptable in the past.
--

Previous thread: [PATCH 2/2] be2net: Fix CSO for UDP packets by Somnath Kotur on Monday, October 25, 2010 - 4:11 am. (2 messages)

Next thread: [PATCH net-next-2.6 2/2] be2net: Schedule/Destroy worker thread in probe()/remove() rather than open()/close() by Somnath Kotur on Monday, October 25, 2010 - 4:13 am. (1 message)