From mboxrd@z Thu Jan  1 00:00:00 1970
From: Thomas Monjalon <thomas.monjalon-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>
Subject: Re: [PATCH v2 05/10] Subjec: igb_uio: msix cleanups
Date: Fri, 18 Jul 2014 13:05:23 +0200
Message-ID: <1912261.SMQix2CgIZ@xps13>
References: <20140606235028.189345212@networkplumber.org>
 <20140606235108.480879979@networkplumber.org>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7Bit
Cc: dev-VfR2kkLFssw@public.gmane.org
To: Stephen Hemminger <stephen-OTpzqLSitTUnbdJkjeBofR2eb7JE58TQ@public.gmane.org>
Return-path: <dev-bounces-VfR2kkLFssw@public.gmane.org>
In-Reply-To: <20140606235108.480879979-OTpzqLSitTUnbdJkjeBofR2eb7JE58TQ@public.gmane.org>
List-Id: patches and discussions about DPDK <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
 <mailto:dev-request-VfR2kkLFssw@public.gmane.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev-VfR2kkLFssw@public.gmane.org>
List-Help: <mailto:dev-request-VfR2kkLFssw@public.gmane.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request-VfR2kkLFssw@public.gmane.org?subject=subscribe>
Errors-To: dev-bounces-VfR2kkLFssw@public.gmane.org
Sender: "dev" <dev-bounces-VfR2kkLFssw@public.gmane.org>

Hi Stephen,

2014-06-06 16:50, Stephen Hemminger:
> Since only one MSI-X entry is ever defined, there is no need to
> put it as an array in the driver private data structure. One msix_entry
> can just be put on the stack and initialized there.

When merging this patch, I realized it's not complete:
an occurence of the msix_entries array is remaining.
See the regarding part of your patch and my proposal below
to be merged in this patch.

> @@ -67,8 +52,6 @@
>  	struct pci_dev *pdev;
>  	spinlock_t lock; /* spinlock for accessing PCI config space or msix data
> in multi tasks/isr */ enum igbuio_intr_mode mode;
> -	struct msix_entry \
> -		msix_entries[IGBUIO_NUM_MSI_VECTORS]; /* pointer to the msix vectors to
> be allocated later */ };
> 
>  static char *intr_mode;
> @@ -526,17 +509,16 @@
> 
>  	/* check if it need to try msix first */
>  	if (igbuio_intr_mode_preferred == IGBUIO_MSIX_INTR_MODE) {
> -		int vector;
> -
> -		for (vector = 0; vector < IGBUIO_NUM_MSI_VECTORS; vector ++)
> -			udev->msix_entries[vector].entry = vector;
> +		/* only one MSIX vector needed */
> +		struct msix_entry msix_entry = {
> +			.entry = 0,
> +		};
> 
> -		if (pci_enable_msix(udev->pdev, udev->msix_entries, IGBUIO_NUM_MSI_VECTORS) == 0) {
> +		if (pci_enable_msix(udev->pdev, &msix_entry, 1) == 0) {
>  			udev->mode = IGBUIO_MSIX_INTR_MODE;
> -		}
> -		else {
> -			pci_disable_msix(udev->pdev);
> -			pr_info("fail to enable pci msix, or not enough msix entries\n");
> +		} else {
> +			pr_err("failed to enable pci msix, or not enough msix entries\n");
> +			udev->mode = IGBUIO_LEGACY_INTR_MODE;
>  		}
>  	}
>  	switch (udev->mode) {


Proposed changes:
- udev->info.irq need to be set with msix_entry
- udev->mode is already the legacy one by default


 		if (pci_enable_msix(udev->pdev, &msix_entry, 1) == 0) {
 			udev->mode = RTE_INTR_MODE_MSIX;
-		} else {
+			udev->info.irq = msix_entry.vector;
+			udev->info.irq_flags = 0;
+		} else
 			pr_err("failed to enable pci msix, or not enough msix entries\n");
-			udev->mode = IGBUIO_LEGACY_INTR_MODE;
-		}
 	}
-	switch (udev->mode) {
-	case RTE_INTR_MODE_MSIX:
-		udev->info.irq_flags = 0;
-		udev->info.irq = udev->msix_entries[0].vector;
-		break;
-	case RTE_INTR_MODE_MSI:
-		break;
-	case RTE_INTR_MODE_LEGACY:
+	if (udev->mode == RTE_INTR_MODE_LEGACY) {
 		udev->info.irq_flags = IRQF_SHARED;
 		udev->info.irq = dev->irq;
-		break;
-	default:
-		break;
 	}


Please confirm it's ok for you.
-- 
Thomas