Re: [PATCH v2 1/2] PCI/MSI: Fix the confusing IRQ sysfs ABI for MSI-X
flat view
From: Bjorn Helgaas <helgaas@kernel.org>
Date: 2021-08-20 23:33:34
Also in:
lkml
[+cc Thomas, Marc] On Sat, Aug 21, 2021 at 10:37:43AM +1200, Barry Song wrote:
From: Barry Song <redacted> /sys/bus/pci/devices/.../irq sysfs ABI is very confusing at this moment especially for MSI-X cases.
AFAICT this patch *only* affects MSI-X. So are you saying the sysfs ABI is fine for MSI but confusing for MSI-X?
quoted hunk ↗ jump to hunk
While MSI sets IRQ to the first number in the vector, MSI-X does nothing for this though it saves default_irq in msix_setup_entries(). Weird the saved default_irq for MSI-X is never used in pci_msix_shutdown(), which is quite different with pci_msi_shutdown(). Thus, this patch moves to show the first IRQ number which is from the first msi_entry for MSI-X. Hopefully, this can make IRQ ABI more clear and more consistent. Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Signed-off-by: Barry Song <redacted> --- drivers/pci/msi.c | 6 ++++++ 1 file changed, 6 insertions(+)diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c index 9232255..6bbf81b 100644 --- a/drivers/pci/msi.c +++ b/drivers/pci/msi.c@@ -771,6 +771,7 @@ static int msix_capability_init(struct pci_dev *dev, struct msix_entry *entries, int ret; u16 control; void __iomem *base; + struct msi_desc *desc; /* Ensure MSI-X is disabled while it is set up */ pci_msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_ENABLE, 0);@@ -814,6 +815,10 @@ static int msix_capability_init(struct pci_dev *dev, struct msix_entry *entries, pci_msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_MASKALL, 0); pcibios_free_irq(dev); + + desc = first_pci_msi_entry(dev); + dev->irq = desc->irq;
This change is not primarily about sysfs. This is about changing
"dev->irq" when MSI-X is enabled, and it's only incidental that sysfs
reflects that.
So we need to know the effect of changing dev->irq. Drivers may use
the value of dev->irq, and I'm *guessing* this change shouldn't break
them since we already do this for MSI, but I'd like some more expert
opinion than mine :)
For MSI we have:
msi_capability_init
msi_setup_entry
entry = alloc_msi_entry(nvec)
entry->msi_attrib.default_irq = dev->irq; /* Save IOAPIC IRQ */
dev->irq = entry->irq;
pci_msi_shutdown
/* Restore dev->irq to its default pin-assertion IRQ */
dev->irq = desc->msi_attrib.default_irq;
and for MSI-X we have:
msix_capability_init
msix_setup_entries
for (i = 0; i < nvec; i++)
entry = alloc_msi_entry(1)
entry->msi_attrib.default_irq = dev->irq;
pci_msix_shutdown
for_each_pci_msi_entry(entry, dev)
__pci_msix_desc_mask_irq
+ dev->irq = entry->msi_attrib.default_irq; # added by this patch
Things that seem strange to me:
- The msi_setup_entry() comment "Save IOAPIC IRQ" seems needlessly
specific; maybe it should be "INTx IRQ".
- The pci_msi_shutdown() comment "Restore ... pin-assertion IRQ"
should match the msi_setup_entry() one, e.g., maybe it should also
be "INTx IRQ". There are no INTx or IOAPIC pins in PCIe.
- The only use of .default_irq is to save and restore dev->irq, so
it looks like a per-device thing, not a per-vector thing.
In msi_setup_entry() there's only one msi_entry, so there's only
one saved .default_irq.
In msix_setup_entries(), we get nvecs msi_entry structs, and we
get a saved .default_irq in each one?
- In pci_msix_shutdown() we restore dev->irq from the .default_irq
of whatever "entry" contains after the for_each_pci_msi_entry()
loop. Is "entry" defined there? I don't know what the cursor
contains after a list_for_each_entry() loop exits.
quoted hunk ↗ jump to hunk
return 0; out_avail:@@ -1024,6 +1029,7 @@ static void pci_msix_shutdown(struct pci_dev *dev) pci_msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_ENABLE, 0); pci_intx_for_msi(dev, 1); dev->msix_enabled = 0; + dev->irq = entry->msi_attrib.default_irq; pcibios_alloc_irq(dev); }-- 1.8.3.1