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

[Bjorn Helgaas]

On Tue, Aug 24, 2021 at 10:46:59AM +1200, Barry Song wrote:
> On Mon, Aug 23, 2021 at 11:28 PM Marc Zyngier <maz@xxxxxxxxxx> wrote:
> >
> > On Mon, 23 Aug 2021 12:03:08 +0100,
> > Barry Song <21cnbao@xxxxxxxxx> wrote:

> +static ssize_t irq_show(struct device *dev,
> +                                        struct device_attribute *attr,
> +                                        char *buf)
> +{
> +       struct pci_dev *pdev = to_pci_dev(dev);
> +#ifdef CONFIG_PCI_MSI
> +       struct msi_desc *desc = first_pci_msi_entry(pdev);
> +
> +       /* for MSI, return the 1st IRQ in IRQ vector */
> +       if (desc && !desc->msi_attrib.is_msix)
> +               return sysfs_emit(buf, "%u\n", desc->irq);
> +#endif
> +
> +       return sysfs_emit(buf, "%u\n", pdev->irq);
> +}
> +static DEVICE_ATTR_RO(irq);

Makes sense to me.  And with Marc's patch maybe we could get rid of
default_irq, which also seems nice.

> > > if we don't want to change the behaviour of any existing ABI, it
> > > seems the only thing we can do here to document it well in ABI
> > > doc. i actually doubt anyone has really understood what the irq
> > > entry is really showing.
> >
> > Given that we can't prove that it is actually the case, I believe this
> > is the only option.
> 
> we have to document the ABI like below though it seems quite annoying.
> 
> 1. for devices which don't support MSI and MSI-X, show legacy INTx
> 2. for devices which support MSI
>     a. if CONFIG_PCI_MSI is not enabled,  show legacy INTx
>     b. if CONFIG_PCI_MSI is enabled and devices are using MSI at this
> moment, show 1st IRQ in the vector
>     c. if CONFIG_PCI_MSI is enabled, but we shutdown its MSI before
> the users call sysfs entry,
>         so at this moment, devices are not using MSI,  show legacy INTx
> 3. for devices which support MSI-X, no matter if it is using MSI-X,
>     show legacy INTx
> 4. In Addition, INTx might be broken due to incomplete firmware or
> hardware design for MSI and MSI-X cases
> 
> To be honest, it sounds like a disaster :-) but if this is what we
> have to do, I'd like to try it in v3.

It doesn't seem necessary to me to get into the gory details of
CONFIG_PCI_MSI -- if that's not enabled, drivers can't use MSI anyway.

I don't understand 3.  If a device supports both MSI and MSI-X and a
driver enables MSI, msi_capability_init() writes dev->irq, so it looks
like "irq" should contain the first MSI vector.

I don't understand 4, either.  Is the possibility of broken hardware
or firmware something we need to document?  

What about something like this?

  If a driver has enabled MSI (not MSI-X), "irq" contains the IRQ of
  the first MSI vector.  Otherwise "irq" contains the IRQ of the
  legacy INTx interrupt.