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

[Bjorn Helgaas]

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

Bjorn

Attachment: irq-range
Description: Binary data