[+cc Thomas, Marc]
On Sat, Aug 21, 2021 at 10:37:43AM +1200, Barry Song wrote:
> From: Barry Song <song.bao.hua@xxxxxxxxxxxxx>
>
> /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?
> 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@xxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Barry Song <song.bao.hua@xxxxxxxxxxxxx>
> ---
> 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.
> 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
>
