Re: PCIe AER generates no interrupts on host (ZynqMP)

[Stefan Roese <sr@denx.de>]

On 1/12/22 18:49, Bjorn Helgaas wrote:
> [+cc Rafael, mentioned 2bd50dd800b5 again]
> 
> On Tue, Jan 11, 2022 at 09:14:13AM +0100, Stefan Roese wrote:
>> On 1/10/22 13:16, Stefan Roese wrote:
>>> On 1/7/22 11:04, Pali Rohár wrote:
>>>> Hello! You asked me in another email for comments to this email, so I'm
>>>> replying directly to this email...
>>>
>>> Thanks a lot for you input here. Please find some comments below...
>>>
>>>> On Tuesday 04 January 2022 10:02:18 Stefan Roese wrote:
>>>>> Hi,
>>>>>
>>>>> I'm trying to get the Kernel PCIe AER infrastructure to work
>>>>> on my ZynqMP based system. E.g. handle the events
>>>>> (correctable, uncorrectable etc). In my current tests, no AER
>>>>> interrupt is generated though. I'm currently using the
>>>>> "surprise down error status" in the uncorrectable error status
>>>>> register of the connected PCIe switch (PLX / Broadcom
>>>>> PEX8718). Here the bit is correctly logged in the PEX switch
>>>>> uncorrectable error status register but no interrupt is
>>>>> generated to the root-port / system. And hence no AER
>>>>> message(s) reported.
>>>>>
>>>>> Does any one of you have some ideas on what might be missing?
>>>>> Why are these events not reported to the PCIe rootport driver
>>>>> via IRQ? Might this be a problem of the missing MSI-X support
>>>>> of the ZynqMP? The AER interrupt is connected as legacy IRQ:
>>>>>
>>>>> cat /proc/interrupts | grep -i aer
>>>>>    58:          0          0          0          0
>>>>> nwl_pcie:legacy   0 Level
>>>>> PCIe PME, aerdrv
>>>>
>>>> Error events (correctable, non-fatal and fatal) are reported by
>>>> PCIe devices to the Root Complex via PCIe error messages
>>>> (Message code of TLP is set to Error Message) and not via
>>>> interrupts. Root Port is then responsible to "convert" these
>>>> PCIe error messages to MSI(X) interrupt and report it to the
>>>> system. According to PCIe spec, AER is supported only via MSI(X)
>>>> interrupts, not legacy INTx.
>>>
>>> This seems not correct. From the ML link reported by Bjorn, there
>>> seems to be a platform specific interrupt on ZynqMP, to report the
>>> AER events.  At least this is how I interpret the patch from
>>> Bharat from that time.  In the meantime Bharat from Xilinx has
>>> sent me a link to a newer, updated patch series to use this "misc"
>>> interrupts for AER instead. I'll get into more details on this in
>>> another reply.
>>>
>>>> Via Bridge Control register (SERR# enable bit) on the Root Port
>>>> it is possible to enable / disable reporting of these errors
>>>> from PCIe devices on the other end of PCIe link to the system.
>>>
>>> Here the BridgeCtl of the Root Port:
>>>     BridgeCtl: Parity- SERR+ NoISA- VGA- VGA16- MAbort- >Reset- FastB2B-
>>>
>>>> Then via Command register (SERR# enable bit) and Device Control
>>>> register it is possible to enable / disable reporting of all
>>>> errors (from Root Port and also devices on other end of the
>>>> link).
>>>
>>> The command registers have SERR disabled on all PCIe devices:
>>>     Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr-
>>> Stepping- SERR- FastB2B- DisINTx-
>>>
>>> Not sure if this is a problem. I would expect the Kernel PCI subsystem
>>> and the AER driver to enable the necessary bits. So is 'SERR-' here
>>> a problem?
>>>
>>> Device Control has the error reporting enabled:
>>>     DevCtl: CorrErr+ NonFatalErr+ FatalErr+ UnsupReq+
>>
>> A small update on this:
>>
>> Just now I noticed, that these bits in the DevCtl register are
>> disabled in the PCIe switch setup downstream and upstream ports.
>> Actually, these bits are only enabled in the root port PCIe device.
>> After enabling these bits via setpci in the PEX switch the surprise
>> down message is reported correctly to the root port:
>>
>>   nwl-pcie fd0e0000.pcie: Fatal Error in AER Capability
>>   pcieport 0000:02:02.0: PCIe Bus Error: severity=Uncorrected (Fatal), type=Transaction Layer, (Receiver ID)
>>   pcieport 0000:02:02.0:   device [10b5:8718] error status/mask=00000020/00000000
>>   pcieport 0000:02:02.0:    [ 5] SDES                   (First)
>>
>> Nice! :)
>>
>> So the question now is, why are these bits in the DevCtl registers of
>> these other PCIe devices disabled? Debugging shows that
>> get_port_device_capability() calls pci_disable_pcie_error_reporting()
>> *after* it has been enabled via the AER driver:
>>
>>   pcieport 0000:00:00.0: get_port_device_capability (225): pcie_ports_native=1 host->native_aer=1
>>   pcieport 0000:00:00.0: AER: pci_disable_pcie_error_reporting (246): rc=0
>>   pcieport 0000:00:00.0: AER: set_device_error_reporting (1218): enable=1
>>   pcieport 0000:00:00.0: AER: pci_enable_pcie_error_reporting (233): rc=0
>>   pci 0000:01:00.0: AER: set_device_error_reporting (1218): enable=1
>>   pci 0000:01:00.0: AER: pci_enable_pcie_error_reporting (233): rc=0
>>   pci 0000:02:01.0: AER: set_device_error_reporting (1218): enable=1
>>   pci 0000:02:01.0: AER: pci_enable_pcie_error_reporting (233): rc=0
>>   pci 0000:02:02.0: AER: set_device_error_reporting (1218): enable=1
>>   pci 0000:02:02.0: AER: pci_enable_pcie_error_reporting (233): rc=0
>>   pci 0000:04:00.0: AER: set_device_error_reporting (1218): enable=1
>>   pci 0000:02:03.0: AER: set_device_error_reporting (1218): enable=1
>>   pci 0000:02:03.0: AER: pci_enable_pcie_error_reporting (233): rc=0
>>   pcieport 0000:01:00.0: get_port_device_capability (225): pcie_ports_native=1 host->native_aer=1
>>   pcieport 0000:01:00.0: AER: pci_disable_pcie_error_reporting (246): rc=0
> 
> Ah.  I assume you have:
> 
>    00:00.0 Root Port to [bus 01-??]
>    01:00.0 Switch Upstream Port to [bus 02-??]
>    02:0?.0 Switch Downstream Port to [bus 04-??]

This is correct, yes.

> pcie_portdrv_probe() claims 00:00.0 and clears CERE NFERE FERE URRE.
> 
> aer_probe() claims 00:00.0 and enables CERE NFERE FERE URRE for all
> downstream devices, including 01:00.0.
> 
> pcie_portdrv_probe() claims 01:00.0 and clears CERE NFERE FERE URRE
> again.
> 
> aer_probe() declines to claim 01:00.0 because it's not a Root Port, so
> CERE NFERE FERE URRE remain cleared.
> 
>>   pcieport 0000:02:01.0: get_port_device_capability (225): pcie_ports_native=1 host->native_aer=1
>>   pcieport 0000:02:01.0: AER: pci_disable_pcie_error_reporting (246): rc=0
>>   pcieport 0000:02:02.0: get_port_device_capability (225): pcie_ports_native=1 host->native_aer=1
>>   pcieport 0000:02:02.0: AER: pci_disable_pcie_error_reporting (246): rc=0
>>   pcieport 0000:02:03.0: get_port_device_capability (225): pcie_ports_native=1 host->native_aer=1
>>   pcieport 0000:02:03.0: AER: pci_disable_pcie_error_reporting (246): rc=0
>>
>> Looking at the comment in get_port_device_capability() this might be the
>> wrong order (e.g. AER driver enabling vs pcieport disabling):
>>
>> static int get_port_device_capability(struct pci_dev *dev)
>> ...
>> 		/*
>> 		 * Disable AER on this port in case it's been enabled by the
>> 		 * BIOS (the AER service driver will enable it when necessary).
>> 		 */
>> 		pci_disable_pcie_error_reporting(dev);
>> 	}
>>
>> I'll look deeper into this today. But perhaps someone else has a quick
>> idea already?
> 
> This was added by 2bd50dd800b5 ("PCI: PCIe: Disable PCIe port services
> during port initialization").  Strangely, this same 11 year old commit
> came up yesterday [1] :)

;)

I'm baffled a bit, that this problem of AER reporting being disabled in
the DevCtl regs of PCIe ports (all non root ports) was not noticed for
this long time. As AER is practically disabled in such setups.

> I'm not 100% convinced that get_port_device_capability() should be
> calling pci_disable_pcie_error_reporting().  IMO, BIOS should not
> leave an interrupt enabled unless it has arranged to handle it.

Agreed.

> It's true that disabling it might work around a BIOS bug.  But the
> usual reason we call pci_disable_pcie_error_reporting() here is
> because host->native_aer is true, and that is usually because
> CONFIG_PCIEAER is set (and, for ACPI systems, _OSC granted us control
> of AER).  That means we're going to call aer_probe(), which should
> enable or disable AER interrupts as it needs.
> 
> So I'm curious whether just removing that call (and removing
> "pcie_ports=native" if you're using it) helps in your case.

Yes, removing the call to pci_disable_pcie_error_reporting() in
get_port_device_capability() helps in my case. DevCtl in the PCIe switch
upstream and downstream ports keeps the AER reporting enabled this way.

> I assume this is probably not an ACPI system, right?  Are you testing
> with Bharat's series [2] applied?

Yes and yes.

I'll work on a v3 of Bharat's series shortly. With your comments from
above, I'll also generate a patch to remove
pci_disable_pcie_error_reporting() from get_port_device_capability().
Let's see, where this does.

Thanks a lot for all your input,
Stefan