[PATCH 2.6.16.18] MSI: Proposed fix for MSI/MSI-X load failure

[PATCH 2.6.16.18] MSI: Proposed fix for MSI/MSI-X load failure

[Ravinandan Arakali]

Hi,
This patch suggests a fix for the MSI/MSI-X load failure.

Please review the patch.

Symptoms:
When a driver is loaded with MSI followed by MSI-X, the load fails indicating 
that the MSI vector is still active. And vice versa.

Suspected rootcause:
This happens inspite of driver calling free_irq() followed by 
pci_disable_msi/pci_disable_msix. This appears to be a kernel bug 
wherein the pci_disable_msi and pci_disable_msix calls do not 
clear/unpopulate the msi_desc data structure that was populated 
by pci_enable_msi/pci_enable_msix.

Proposed fix:
Free the MSI vector in pci_disable_msi and all allocated MSI-X vectors 
in pci_disable_msix.

Testing:
The fix has been tested on IA64 platforms with Neterion's Xframe driver.

Signed-off-by: Ravinandan Arakali <ravinandan.arakali@neterion.com>
---

diff -urpN old/drivers/pci/msi.c new/drivers/pci/msi.c
--- old/drivers/pci/msi.c	2006-05-31 19:02:19.000000000 -0700
+++ new/drivers/pci/msi.c	2006-05-31 19:02:39.000000000 -0700
@@ -779,6 +779,7 @@ void pci_disable_msi(struct pci_dev* dev
 		nr_released_vectors++;
 		default_vector = entry->msi_attrib.default_vector;
 		spin_unlock_irqrestore(&msi_lock, flags);
+		msi_free_vector(dev, dev->irq, 1);
 		/* Restore dev->irq to its default pin-assertion vector */
 		dev->irq = default_vector;
 		disable_msi_mode(dev, pci_find_capability(dev, PCI_CAP_ID_MSI),
@@ -1046,6 +1047,7 @@ void pci_disable_msix(struct pci_dev* de
 
 		}
 	}
+	msi_remove_pci_irq_vectors(dev);
 }
 
 /**

Re: [PATCH 2.6.16.18] MSI: Proposed fix for MSI/MSI-X load failure

[Rajesh Shah]

On Fri, Jun 02, 2006 at 03:21:37PM -0400, Ravinandan Arakali wrote:
> 
> Symptoms:
> When a driver is loaded with MSI followed by MSI-X, the load fails indicating 
> that the MSI vector is still active. And vice versa.
> 
> Suspected rootcause:
> This happens inspite of driver calling free_irq() followed by 
> pci_disable_msi/pci_disable_msix. This appears to be a kernel bug 
> wherein the pci_disable_msi and pci_disable_msix calls do not 
> clear/unpopulate the msi_desc data structure that was populated 
> by pci_enable_msi/pci_enable_msix.
> 
The current MSI code actually does this deliberately, not by
accident. It's got a lot of complex code to track devices and
vectors and make sure an enable_msi -> disable -> enable sequence
gives a driver the same vector. It also has policies about
reserving vectors based on potential hotplug activity etc.
Frankly, I've never understood the need for such policies, and
am in the process of removing all of them.

> Proposed fix:
> Free the MSI vector in pci_disable_msi and all allocated MSI-X vectors 
> in pci_disable_msix.
> 
This will break the existing MSI policies. Once you take that away,
a whole lot of additional code and complexity can be removed too.
That's what I'm working on right now, but such a change is likely
too big for -stable.

So, I'm ok with this patch if it actually doesn't break MSI/MSI-X.
Did you try to repeatedly load/unload an MSI capable driver with
this patch? Did you repeatedly try to ifdown/ifup an Ethernet
driver that uses MSI? I'm not in a position to test this today, but
will try it out next week.

thanks,
Rajesh

RE: [PATCH 2.6.16.18] MSI: Proposed fix for MSI/MSI-X load failure

[Ravinandan Arakali]

Rajesh,
It's possible that the current behavior is by design but once the driver is loaded with MSI, you need a reboot to be able to load MSI-X. And vice versa. I found this rather restrictive.

I did test the fix multiple times. For eg. multiple load/unload iterations of
MSI followed by multiple load/unload of MSI-X followed by load/unload MSI. That way both transitions(MSI-to-MSI-X and vice versa) are tested.

Thanks,
Ravi

-----Original Message-----
From: Rajesh Shah [mailto:rajesh.shah@intel.com]
Sent: Friday, June 02, 2006 2:55 PM
To: Ravinandan Arakali
Cc: linux-kernel@vger.kernel.org; netdev@vger.kernel.org; Leonid
Grossman; Ananda Raju; Sriram Rapuru
Subject: Re: [PATCH 2.6.16.18] MSI: Proposed fix for MSI/MSI-X load
failure


On Fri, Jun 02, 2006 at 03:21:37PM -0400, Ravinandan Arakali wrote:
> 
> Symptoms:
> When a driver is loaded with MSI followed by MSI-X, the load fails indicating 
> that the MSI vector is still active. And vice versa.
> 
> Suspected rootcause:
> This happens inspite of driver calling free_irq() followed by 
> pci_disable_msi/pci_disable_msix. This appears to be a kernel bug 
> wherein the pci_disable_msi and pci_disable_msix calls do not 
> clear/unpopulate the msi_desc data structure that was populated 
> by pci_enable_msi/pci_enable_msix.
> 
The current MSI code actually does this deliberately, not by
accident. It's got a lot of complex code to track devices and
vectors and make sure an enable_msi -> disable -> enable sequence
gives a driver the same vector. It also has policies about
reserving vectors based on potential hotplug activity etc.
Frankly, I've never understood the need for such policies, and
am in the process of removing all of them.

> Proposed fix:
> Free the MSI vector in pci_disable_msi and all allocated MSI-X vectors 
> in pci_disable_msix.
> 
This will break the existing MSI policies. Once you take that away,
a whole lot of additional code and complexity can be removed too.
That's what I'm working on right now, but such a change is likely
too big for -stable.

So, I'm ok with this patch if it actually doesn't break MSI/MSI-X.
Did you try to repeatedly load/unload an MSI capable driver with
this patch? Did you repeatedly try to ifdown/ifup an Ethernet
driver that uses MSI? I'm not in a position to test this today, but
will try it out next week.

thanks,
Rajesh

Re: [PATCH 2.6.16.18] MSI: Proposed fix for MSI/MSI-X load failure

[Paul Mackerras]

Rajesh Shah writes:

> The current MSI code actually does this deliberately, not by
> accident. It's got a lot of complex code to track devices and
> vectors and make sure an enable_msi -> disable -> enable sequence
> gives a driver the same vector. It also has policies about
> reserving vectors based on potential hotplug activity etc.
> Frankly, I've never understood the need for such policies, and
> am in the process of removing all of them.

Good.  We will not be able to support a policy of giving the driver
the same vector across an enable_msi/disable/enable sequence on IBM
System p machines (64-bit PowerPC), because the firmware controls the
MSI allocation, and it doesn't give us the necessary guarantees.

Paul.