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
