Re: [Bug 215560] New: _PRS/_SRS methods should be optional

[Pierre Gondois <pierre.gondois@arm.com>]

On 2/3/22 5:32 PM, Bjorn Helgaas wrote:
> [+cc Rafael, Alex, Marc]
> 
> On Thu, Feb 03, 2022 at 10:10:19AM +0100, Pierre Gondois wrote:
>> On 2/2/22 6:42 PM, Bjorn Helgaas wrote:
>>> On Wed, Feb 02, 2022 at 10:20:44AM +0000, bugzilla-daemon@kernel.org wrote:
>>>> https://bugzilla.kernel.org/show_bug.cgi?id=215560
>>>>
>>>> The PCI legacy interrupts can be described with link devices, cf ACPI 6.4,
>>>> s6.2.13 "_PRT (PCI Routing Table)".
>>>> Link devices can have optional _SRS/_PRS methods to set the interrupt.
>>>> ...
> 
>>>> However, _PRS/_SRS methods are checked in drivers/acpi/pci_link.c, and the
>>>> driver aborts if they are absent.
>>>> E.g.: When _PRS is missing:
>>>> ACPI: \_SB_.PCI0.LNKA: _CRS 36 not found in _PRS
>>>> ACPI: \_SB_.PCI0.LNKA: No IRQ available. Try pci=noacpi or acpi=off
>>>
>>> I assume this bug report is because something isn't working.  Can
>>> you update the bugzilla with a note about what specifically isn't
>>> working and also attach a complete dmesg log and acpidump?
>>
>> The question arose while writing link devices code, so there is no
>> platform with missing _PRS/_SRS methods that I know.
>>
>> The question was more about spec compliance and the necessity to
>> have these methods when legacy interrupts are not configurable.  The
>> message above (_CRS XXX not found in _PRS) can be generated for a
>> Juno for instance, and the ACPI tables are at:
>> https://github.com/tianocore/edk2-platforms/blob/master/Platform/ARM/JunoPkg/AcpiTables/AcpiSsdtRootPci.asl
>> The ACPI table need to be modified (remove _PRS and set _CRS
>> correctly).
>>
>> If the conclusion is that _PRS/_SRS are mandatory, even for
>> hard-wired interrupts, then the bugzilla can be closed.
> 
> OK, so if I understand correctly, you're using Interrupt Link devices
> not because it's possible to connect a PCI interrupt to one of several
> inputs on the interrupt controller, but because the PCI default of
> "level triggered, active low" is not compatible with GICv2.
> 
> The Interrupt Link device gives you a chance to specify "level
> triggered, active *high*".  If you used a Source of 0 (where there
> is no Interrupt Link), there would be no way to specify that.
> 
> Since this use of Interrupt Links only conveys triggering information
> and nothing is configurable, I think the OS should get that info from
> _CRS, and _PRS and _SRS should not be required.
> 
> Alex made a change [1] along that line a while ago, but maybe there's
> more we should do.
> 
> Bjorn
> 
> [1] https://git.kernel.org/linus/92d1b381f677
> 

Yes, this is exactly the situation.

The interrupt advertised in _CRS is checked to be in _PRS at:
https://github.com/torvalds/linux/blob/26291c54e111ff6ba87a164d85d4a4e134b7315c/drivers/acpi/pci_link.c#L549
and the _SRS method is also evaluated.

I can submit a patch if necessary,
Pierre