Re: [PATCH] pci: do not try to assign irq 255

From: Hannes Reinecke
Date: Thu Feb 28 2013 - 11:13:37 EST

On 02/27/2013 10:13 PM, Bjorn Helgaas wrote:
[+cc Andy]

On Wed, Feb 20, 2013 at 11:53 PM, Hannes Reinecke <hare@xxxxxxx> wrote:
On 02/20/2013 05:57 PM, Yinghai Lu wrote:

On Tue, Feb 19, 2013 at 11:58 PM, Hannes Reinecke <hare@xxxxxxx> wrote:


Apparently this device is meant to use MSI _only_ so the BIOS developer
didn't feel the need to assign an INTx here.

According to PCI-3.0, section 6.8 (Message Signalled Interrupts):

It is recommended that devices implement interrupt pins to
provide compatibility in systems that do not support MSI
(devices default to interrupt pins). However, it is expected
that the need for interrupt pins will diminish over time.
Devices that do not support interrupt pins due to pin
constraints (rely on polling for device service) may implement
messages to increase performance without adding additional pins. >
Therefore, system configuration software must not assume that a
message capable device has an interrupt pin.


Which sounds to me as if the implementation is valid...


it seems you mess pin with interrupt line.

current code:
unsigned char irq;

pci_read_config_byte(dev, PCI_INTERRUPT_PIN, &irq);
dev->pin = irq;
if (irq)
pci_read_config_byte(dev, PCI_INTERRUPT_LINE, &irq);
dev->irq = irq;

so if the device does not have interrupt pin implemented, pin should be
zero.
and pin and irq in dev should
be all 0.

But the device _has_ an interrupt pin implemented.
The whole point here is that the interrupt line is _NOT_ zero.

00:14.0 USB controller [0c03]: Intel Corporation 7 Series/C210 Series
Chipset Family USB xHCI Host Controller [8086:1e31] (rev 04) (prog-if 30
[XHCI])
Subsystem: Hewlett-Packard Company Device [103c:179b]
Control: I/O- Mem+ BusMaster- SpecCycle- MemWINV- VGASnoop- ParErr-
Stepping- SERR- FastB2B- DisINTx-
Status: Cap+ 66MHz- UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort-
<TAbort- <MAbort- >SERR- <PERR- INTx-
Interrupt: pin A routed to IRQ 255
Region 0: Memory at d4720000 (64-bit, non-prefetchable) [size=64K]
Capabilities: [70] Power Management version 2
Flags: PMEClk- DSI- D1- D2- AuxCurrent=375mA
PME(D0-,D1-,D2-,D3hot+,D3cold+)
Status: D0 NoSoftRst+ PME-Enable- DSel=0 DScale=0 PME-
Capabilities: [80] MSI: Enable- Count=1/8 Maskable- 64bit+
Address: 0000000000000000 Data: 0000

So at one point we have to decide that ->irq is not valid, despite it being
not set to zero.
An alternative fix would be this:

diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c
index 68a921d..4a480cb 100644
--- a/drivers/acpi/pci_irq.c
+++ b/drivers/acpi/pci_irq.c
@@ -469,6 +469,7 @@ int acpi_pci_irq_enable(struct pci_dev *dev)
} else {
dev_warn(&dev->dev, "PCI INT %c: no GSI\n",
pin_name(pin));
+ dev->irq = 0;
}
return 0;
}

Which probably is a better solution, as here ->irq is _definitely_
not valid, so we should reset it to '0' to avoid confusion on upper
layers.

I didn't like the pci_read_irq() change because the PCI spec doesn't
say anything about any PCI_INTERRUPT_LINE values being invalid.

I like this solution better, but I still don't quite understand it.
We have the following code in acpi_pci_irq_enable(). We have
previously tried to look up "gsi," but the _PRT doesn't mention this
device, so we have "gsi == -1" at this point:

/*
* No IRQ known to the ACPI subsystem - maybe the BIOS /
* driver reported one, then use it. Exit in any case.
*/
if (gsi < 0) {
u32 dev_gsi;
/* Interrupt Line values above 0xF are forbidden */
if (dev->irq > 0 && (dev->irq <= 0xF) &&
(acpi_isa_irq_to_gsi(dev->irq, &dev_gsi) == 0)) {
dev_warn(&dev->dev, "PCI INT %c: no GSI -
using ISA IRQ %d\n",
pin_name(pin), dev->irq);
acpi_register_gsi(&dev->dev, dev_gsi,
ACPI_LEVEL_SENSITIVE,
ACPI_ACTIVE_LOW);
} else {
dev_warn(&dev->dev, "PCI INT %c: no GSI\n",
pin_name(pin));
}

return 0;
}

1) I don't know where the restriction of 0x1-0xF came from.
Presumably this value of dev->irq came from PCI_INTERRUPT_LINE, and I
don't know what forbids values > 0xF. The test was added by Andy
Grover in the attached commit. This is ancient history; probably Andy
doesn't remember either :)

2) The proposed change (setting "dev->irq = 0" when we didn't find
anything in the _PRT and dev->irq > 0xF) throws away some information,
and I fear it could break systems. For example, what would happen if
a system put an IOAPIC pin number in a device's PCI_INTERRUPT_LINE and
omitted the device from _PRT? Is it possible the device would still
work as-is (with acpi_pci_irq_enable() doing nothing), but would break
if we set dev->irq = 0?

3) I don't understand why the xhci init fails in the first place. It
looks like the "request interrupt 255 failed" message is from
xhci_try_enable_msi(), but that function tries to enable MSI-X, then
MSI, then falls back to legacy interrupts, where we get the error.
But the device supports MSI, so I don't know why we even fall back to
trying legacy interrupts. Hannes, do you have any insight into that?
Obviously I'm missing something here.

Right.

It's infact a quirk with the USB HCD setup.

drivers/usb/core/hcd.c calls request_irq for the legacy interrupt line, before drivers/usb/host/xhci.c enables MSI-X.

So the correct thing would be to skip the request_irq() function in drivers/usb/core/hcd.c.

I'll be drafting up a patch.

Cheers,

Hannes
--
Dr. Hannes Reinecke zSeries & Storage
hare@xxxxxxx +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/