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-24 19:34:43
Also in:
lkml
On Tue, Aug 24, 2021 at 10:46:59AM +1200, Barry Song wrote:
On Mon, Aug 23, 2021 at 11:28 PM Marc Zyngier [off-list ref] wrote:quoted
On Mon, 23 Aug 2021 12:03:08 +0100, Barry Song [off-list ref] wrote:
quoted hunk ↗ jump to hunk
+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.
quoted
quoted
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.