Re: [PATCH v2 05/10] Subjec: igb_uio: msix cleanups

[Thomas Monjalon <thomas.monjalon-pdR9zngts4EAvxtiuMwx3w@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