Re: Defining polarity and trigger mode for static interrupts in _PRT

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Aug 25, 2016 at 4:14 AM, Lorenzo Pieralisi
<lorenzo.pieralisi@xxxxxxx> wrote:
> On Thu, Aug 25, 2016 at 11:04:02AM +0100, Punit Agrawal wrote:
>> Marc Zyngier <marc.zyngier@xxxxxxx> writes:
>>
>> > On Wed, 24 Aug 2016 14:30:00 -0500
>> > Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
>> >
>> >> On Wed, Aug 24, 2016 at 03:27:23PM +0100, Lorenzo Pieralisi wrote:
>> >> > [ +Bjorn, Punit]
>> >> >
>> >> > On Wed, Aug 24, 2016 at 04:06:13AM -0700, Duc Dang wrote:
>> >> > > [Resend in plain text mode]
>> >> > >
>> >> > > Hi Lorenzo, Rafael,
>> >> > >
>> >> > > ACPI 6.1 spec does not specify how to set interrupt polarity and
>> >> > > trigger mode in _PRT when the interrupts are static (hardwired to
>> >> > > specific interrupt inputs in interrupt controller). In current
>> >> > > acpi_pci_irq_enable (drivers/acpi/pci_irq.c) implementation, by
>> >> > > default the trigger mode is set to LEVEL_SENSITIVE, polarity is set to
>> >> > > ACTIVE_LOW. This default setting won't work for ARM64 GICv2, GICv2m,
>> >> > > GICv3 controllers and will cause failures in PCIe AER, PME services
>> >> > > (on X-Gene platforms).
>> >>
>> >> PCI (not PCIe) r3.0, sec 2.2.6, says "Interrupts on PCI are optional
>> >> and defined as 'level sensitive,' asserted low."
>> >>
>> >> I've heard before that ARM64 does this differently, but I still don't
>> >> understand the difference.  Obviously if you plug a legacy PCI card
>> >> into an ARM64 system, it's still going to pull INTA# low to assert an
>> >> interrupt.  So is there something special about ARM64 that inverts
>> >> that, or what?
>> >
>> > There is certainly an inverter somewhere on the interrupt path, because
>> > the GIC triggers on level high, not level low. But I don't think that's
>> > the issue Duc is trying to outline here, because that's not something
>> > SW can fix. I'm worried that in his system, the interrupt is edge
>> > triggered instead.
>
> It would be nice if Duc reported the "failures" he is noticing,
> instead of us having to guess them.

I am sorry that I should be more specific. Please see failure information below.

>
>> >> > > Is there any way to specify polarity and trigger mode for static
>> >> > > interrupts in _PRT?
>> >>
>> >> There is no way I'm aware of in _PRT to specify polarity and trigger
>> >> mode.  I don't know the history, but my guess is that it would be seen
>> >> as superfluous given that the PCI spec requires level, active low.
>> >>
>> >> Obviously I'm missing something important.
>> >
>> > Same here, unless the HW is not PCI compliant...
>>
>> I had faced this issue on Juno r2[0] a few months back - though AFAICS,
>> it wasn't preventing anything to work but printed an annoying message on
>> boot.
>>
>> [ 1.353696] genirq: Setting trigger mode 8 for irq 9 failed (gic_set_type+0x0/0x5c)
>> [ 1.478286] genirq: Setting trigger mode 8 for irq 17 failed (gic_set_type+0x0/0x5c)

I got the same warning:
[    6.576842] genirq: Setting trigger mode 8 for irq 50 failed
(gic_set_type+0x0/0x64)

>
> This is not just annoying, that's the kernel failing to set-up legacy
> IRQs IIUC and unless we have a way to specify interrupt polarity that's
> going to happen on all ARM64 platforms booting with ACPI having a GIC
> interrupt controller.

The warning about gic_set_type above also happens with previous
kernel, but INTx interrupt handler is still registered successfully,
so things that use INTx still work. I dump a trace of PCIe AER service
initialization  in 4.6 and 4.8-rc1 and the code path leading to
gic_set_type is different: in 4.8-rc1, request_irq fails when
gic_set_type fails and the handler cannot be registered:

4.6-rc2 code path:
[    6.455289] [<ffff0000083624d0>] gic_set_type+0x50/0x64
[    6.460943] [<ffff0000080fb984>] __irq_set_trigger+0x58/0x178
[    6.467176] [<ffff0000080fd0e8>] irq_set_irq_type+0x30/0x5c
[    6.473169] [<ffff000008100dcc>] irq_create_fwspec_mapping+0x194/0x1e4
[    6.480121] [<ffff0000083d5368>] acpi_register_gsi+0x54/0x5c
[    6.486303] [<ffff0000083d2660>] acpi_pci_irq_enable+0xc8/0x14c
[    6.492771] [<ffff00000809382c>] pcibios_alloc_irq+0x20/0x58
[    6.498825] [<ffff000008398928>] pci_device_probe+0x28/0x108
[    6.504844] [<ffff000008481a20>] driver_probe_device+0x200/0x2a0
[    6.511241] [<ffff000008481b6c>] __driver_attach+0xac/0xb0
[    6.517138] [<ffff00000847fa90>] bus_for_each_dev+0x58/0x98
[    6.523138] [<ffff000008481330>] driver_attach+0x20/0x28
[    6.528792] [<ffff000008480f48>] bus_add_driver+0x1c8/0x22c
[    6.534741] [<ffff000008482424>] driver_register+0x60/0xf4
[    6.540594] [<ffff0000083977b8>] __pci_register_driver+0x40/0x48
[    6.546993] [<ffff000008b4fb20>] pcie_portdrv_init+0x84/0xa4
[    6.553175] [<ffff0000080829d8>] do_one_initcall+0x8c/0x19c
[    6.559176] [<ffff000008b2eb00>] kernel_init_freeable+0x150/0x1f0
[    6.565748] [<ffff0000087f05c0>] kernel_init+0x10/0xfc
[    6.571213] [<ffff000008085e10>] ret_from_fork+0x10/0x40
[    6.576842] genirq: Setting trigger mode 8 for irq 50 failed
(gic_set_type+0x0/0x64)
[    6.585689] aer 0000:00:00.0:pcie02: service driver aer loaded


4.8-rc1 code path:
[    5.829216] [<ffff000008384fb4>] gic_set_type+0x60/0x74
[    5.834778] [<ffff0000080fe344>] __irq_set_trigger+0x58/0x178
[    5.840890] [<ffff0000080fea3c>] __setup_irq+0x5d8/0x65c
[    5.846536] [<ffff0000080fec9c>] request_threaded_irq+0x114/0x1c4
[    5.853038] [<ffff0000083db4bc>] aer_probe+0xd4/0x274
[    5.858418] [<ffff0000083d9064>] pcie_port_probe_service+0x38/0x80
[    5.865007] [<ffff0000084c36b0>] driver_probe_device+0x200/0x2a0
[    5.871378] [<ffff0000084c37fc>] __driver_attach+0xac/0xb0
[    5.877215] [<ffff0000084c1720>] bus_for_each_dev+0x58/0x98
[    5.883138] [<ffff0000084c2fc0>] driver_attach+0x20/0x28
[    5.888800] [<ffff0000084c2bd8>] bus_add_driver+0x1c8/0x22c
[    5.894749] [<ffff0000084c40b4>] driver_register+0x60/0xf4
[    5.900611] [<ffff0000083d8fb0>] pcie_port_service_register+0x58/0x68
[    5.907477] [<ffff000008ce3dd8>] aer_service_init+0x30/0x38
[    5.913400] [<ffff000008083334>] do_one_initcall+0x38/0x128
[    5.919323] [<ffff000008cc0cc8>] kernel_init_freeable+0x150/0x1f0
[    5.925809] [<ffff0000088f4278>] kernel_init+0x10/0xfc
[    5.931239] [<ffff000008082e90>] ret_from_fork+0x10/0x40
[    5.936913] genirq: Setting trigger mode 8 for irq 50 failed
(gic_set_type+0x0/0x74)
[    5.945163] aer 0000:00:00.0:pcie002: request IRQ failed
[    5.950844] aer: probe of 0000:00:00.0:pcie002 failed with error -22

>
> I suspect most of ARM partners are already using interrupt links to
> work around this and Duc was the first one reporting the issue
> with the ACPI specs (ie interrupt links should just be used for
> configurable interrupt pins - which, I will say it again - it is
> likely to be a consequence of how x86 and their APIC works).

I tried your suggestion about using interrupt links and it works. But
unfortunately, it requires firmware change.

>
>> The ACPI code in the kernel (drivers/acpi/pci_irq.c) is behaving as per
>> spec, so nothing to be done there IMHO. The problem arises due to the
>> integration of two mismatched components - PCI is level low and the GIC
>> supports only level high - making it necessary to introduce glue
>> elements like the inverter.
>
> That glue can well be the interrupt link. I reiterate my point:
> I will start dumping x86 ACPI tables and peruse the _PRT on x86 systems,
> but in principle even on x86 the legacy PCI interrupt can be re-routed
> and their polarity specified through interrupt links. It is not a
> given that the legacy IRQs are active low at interrupt controller
> level even on x86, at least that's not ruled out by ACPI specs.
>
> Am I wrong ?
>
> The inverter you and Marc mentioned can be described through an
> interrupt link defining the interrupt polarity. On x86 there is
> probably an IRQ router sitting downstream the ACPI that allows
> to a) configure the IOAPIC pin for the legacy IRQ and b) control
> the polarity, but as I mentioned I do not have enough x86 knowledge,
> so I am just guessing.
>
>> This would all be OK if ACPI had a mechanism to specify the interrupt
>> type (trigger and polarity). As an alternative, for Juno I created a
>> link device (as Lorenzo suggests) to provide this information to the
>> kernel.
>>
>> With this fix the warnings went away and I suspect this will address
>> Duc's issue as well. But that is playing naughty with the spec (ACPI 6.1
>> Section. 6.2.13).
>
> ACPI specs were written for x86 boxes, there is nothing naughty
> in asking for them to be updated and work on other architectures.
>
> The only issue I see with interrupt links is that they may allow
> reprogramming through the _SRS method, which is optional BTW.
>
>> If there are no good reason to restrict using link devices to
>> configurable interrupts, perhaps the spec needs an update.
>
> Yes and that's what I am going to ask if nobody complains.

In my opinion, if the interrupt is not configurable, I should be able
to choose not to use interrupt link, which requires a spec. update as
well to specify the polarity of the fixed interrupts?

>
>> Perhaps Rafael knows why is there a restriction on using link devices
>> for fixed interrupts in the ACPI spec...
>
> See above, I do not know why that restriction is there given that
> to me an interrupt link is a superset of static values, I do not
> see why they should be prevented for non-configurable interrupts,
> I am happy to be corrected if there is something we are missing.
>
> Lorenzo

Regards,
Duc Dang.
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux