pci: return 0 for pci_remap_intr_method MSI-X non-error case

[emaste]

Summary

When remapping a MSI-X vector, we would always return ENOENT, even if
successful. In practice this is harmless since the sole caller of
BUS_REMAP_INTR doesn't check for errors (and failure is not likely to
result in a functional issue).

Return 0 if there's no error, so that we can start printing a warning
in msi_assign_cpu in the case that pci_remap_intr_method actually fails.

[emaste]

sys/dev/pci/pci.c

2454

presumably it's not possible to have the same IRQ in multiple vectors?

[emaste]

the sole caller of BUS_REMAP_INTR currently doesn't check for errors.

I've done this in my wip tree for now:

diff --git a/sys/x86/x86/msi.c b/sys/x86/x86/msi.c
index 8d66dcdf7d84..50f679f8ad40 100644
--- a/sys/x86/x86/msi.c
+++ b/sys/x86/x86/msi.c
@@ -246,7 +246,7 @@ msi_assign_cpu(struct intsrc *isrc, u_int apic_id)
        struct msi_intsrc *sib, *msi = (struct msi_intsrc *)isrc;
        int old_vector;
        u_int old_id;
-       int i, vector;
+       int error, i, vector;
 
        /*
         * Only allow CPUs to be assigned to the first message for an
@@ -305,8 +305,11 @@ msi_assign_cpu(struct intsrc *isrc, u_int apic_id)
                    "msi: Assigning MSI IRQ %d to local APIC %u vector %u\n",
                            sib->msi_irq, sib->msi_cpu, sib->msi_vector);
        }
-       BUS_REMAP_INTR(device_get_parent(msi->msi_dev), msi->msi_dev,
+       error = BUS_REMAP_INTR(device_get_parent(msi->msi_dev), msi->msi_dev,
            msi->msi_irq);
+       if (error != 0)
+               printf("%s: error %d from BUS_REMAP_INTR for irq %u\n",
+                   __func__, error, msi->msi_irq);
 
        /*
         * Free the old vector after the new one is established.  This is done

[markj]

sys/dev/pci/pci.c

2454

Based on pci_alloc_msix_method(), I don't believe so, no.

[emaste]

Based on pci_alloc_msix_method(), I don't believe so, no.

LGTM then. I think it's worth noting this in the commit message.

[jhb]

The right fix is to add a 'bool found = false' at the start of the loop and reset it to true where you have currently added the new return, and change the final return value to be conditional on found.

sys/dev/pci/pci.c

2454

Err, no, it is certainly possible. You have to call pci_remap_msix to do it, but it is supported. That's also why the comment above this loop says "all the slots that use this IRQ" instead of "find the one slot that uses this IRQ".

[jhb]

sys/dev/pci/pci.c

2474

This is the one I think you would want to change to return (found ? 0 : ENOENT);

[emaste]

Actually not harmless as we remove the old vector in msi_assign_cpu even if BUS_REMAP_INTR failed. Will just remove the "harmless" sentence from the commit message.