On Mon, 25 Jan 2016, Bjorn Helgaas wrote:quoting the spec document:
On Mon, Jan 25, 2016 at 02:59:38PM +0800, Chen Fan wrote:The current code does not not fail when the interrupt request fails. It
i801_smbus 0000:00:1f.3: PCI INT C: no GSI
i801_smbus 0000:00:1f.3: Failed to allocate irq 255: -16
i801_smbus: probe of 0000:00:1f.3 failed with error -16
reports it and clears the IRQ feature flag.
And why would this be x86 specific? PCI3.0 is architecture independent, right?@@ -436,7 +437,15 @@ int acpi_pci_irq_enable(struct pci_dev *dev)
* driver reported one, then use it. Exit in any case.
*/
if (gsi < 0) {
- if (acpi_isa_register_gsi(dev))
+#ifdef CONFIG_X86
+ /*
+ * The Interrupt Line value of 0xff is defined to mean "unknown"
+ * or "no connection" (PCI 3.0, Section 6.2.4, footnote on page
+ * 223), using ~0U as invalid IRQ.
+ */
yes, this is what I thought in previous email, I has asked that
I do not understand that IRQ_INVALID business at all.+ dev->irq = (dev->irq == 0xff) ? IRQ_INVALID : dev->irq;It's much simpler and clearer to write:
if (dev->irq == 0xff)
dev->irq = IRQ_INVALID;
The existing code already drops into this place because+#endif
+ if (!irq_is_valid(dev->irq) || acpi_isa_register_gsi(dev))
dev_warn(&dev->dev, "PCI INT %c: no GSI\n",
pin_name(pin));
acpi_isa_register_gsi() fails.
What extra value does that !irq_is_valid() provide?i801_smbus 0000:00:1f.3: PCI INT C: no GSI
And how does setting dev->irq to ~0U prevent that request_irq() is called in
the i801 device driver? Not at all, AFAICT. It will just fail with a different
error.
So the whole 'fix' relies on the fact that irq ~0U does not exist (at least
not today) and therefor the false sharing with some other driver using irq 255
will not happen.
Relying on undocumented behaviour is not a fix, that's voodoo programming.
The proper solution here is to flag that this device does not have an
interrupt connected and act accordingly in the device driver, i.e. do not call
request_irq() in the first place.
No. NR_IRQS cannot be used at all if sparse irqs are enabled. Nothing in any+static inline bool irq_is_valid(unsigned int irq)I don't like the x86 ifdef. I'd prefer:
+{
+#ifdef CONFIG_X86
+ if (irq == IRQ_INVALID)
+ return false;
+#endif
+ return true;
+}
static inline bool irq_valid(unsigned int irq)
{
if (irq < NR_IRQS)
return true;
return false;
}
This could be used in many of the places that currently use NR_IRQS.
generic code is supposed to rely on NR_IRQS.
Thanks,
tglx
.