Patchwork [RFC,v2,06/29] PCI/MSI: Get rid of useless count of msi_desc leftovers

login
register
mail settings
Submitter Alexander Gordeev
Date Oct. 18, 2013, 5:12 p.m.
Message ID <79a2aeb6a06563c5f09463d53d2e0146e44c92be.1382103786.git.agordeev@redhat.com>
Download mbox | patch
Permalink /patch/284827/
State Superseded
Headers show

Comments

Alexander Gordeev - Oct. 18, 2013, 5:12 p.m.
If an architecture failed to allocate requested number of
MSI-Xs we should not mislead device drivers and make them
retry based on how many MSI-Xs were possibly allocated by
the architecture in the process, before the failure. It is
rude for an architecture to leave any leftovers anyway.

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 drivers/pci/msi.c |   19 +------------------
 1 files changed, 1 insertions(+), 18 deletions(-)
Tejun Heo - Nov. 20, 2013, 4:07 p.m.
Hello, Alexander.

On Fri, Oct 18, 2013 at 07:12:06PM +0200, Alexander Gordeev wrote:
> @@ -744,23 +744,6 @@ static int msix_capability_init(struct pci_dev *dev,
>  
>  	return 0;
>  
> -out_avail:
> -	if (ret < 0) {
> -		/*
> -		 * If we had some success, report the number of irqs
> -		 * we succeeded in setting up.
> -		 */
> -		struct msi_desc *entry;
> -		int avail = 0;
> -
> -		list_for_each_entry(entry, &dev->msi_list, list) {
> -			if (entry->irq != 0)
> -				avail++;
> -		}
> -		if (avail != 0)
> -			ret = avail;
> -	}

Hmmm... so, before this, the function would have returned the partial
number of irqs that arch set up successfully.  Is it okay to lose that
information?  If so, can you please elaborate a bit more on why it's
okay in the description?

Thanks.
Alexander Gordeev - Nov. 22, 2013, 6:27 p.m.
On Wed, Nov 20, 2013 at 11:07:16AM -0500, Tejun Heo wrote:
> Hmmm... so, before this, the function would have returned the partial
> number of irqs that arch set up successfully.  Is it okay to lose that
> information?  If so, can you please elaborate a bit more on why it's
> okay in the description?

Well, this is embarrassing. A quick check into x86 shows the partially
initialized MSI-Xs are possible - something I should have noticed.

Self-Nack.

> tejun

Patch

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index b43f391..bbe3d3d 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -719,7 +719,7 @@  static int msix_capability_init(struct pci_dev *dev,
 
 	ret = arch_setup_msi_irqs(dev, nvec, PCI_CAP_ID_MSIX);
 	if (ret)
-		goto out_avail;
+		goto out_free;
 
 	/*
 	 * Some devices require MSI-X to be enabled before we can touch the
@@ -744,23 +744,6 @@  static int msix_capability_init(struct pci_dev *dev,
 
 	return 0;
 
-out_avail:
-	if (ret < 0) {
-		/*
-		 * If we had some success, report the number of irqs
-		 * we succeeded in setting up.
-		 */
-		struct msi_desc *entry;
-		int avail = 0;
-
-		list_for_each_entry(entry, &dev->msi_list, list) {
-			if (entry->irq != 0)
-				avail++;
-		}
-		if (avail != 0)
-			ret = avail;
-	}
-
 out_free:
 	free_msi_irqs(dev);