Re: [PATCH v2 1/2] PCI/MSI: Fix the confusing IRQ sysfs ABI for MSI-X

[Bjorn Helgaas]

[+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
>