PCIe AER generates no interrupts on host (ZynqMP)

PCIe AER generates no interrupts on host (ZynqMP)

[Stefan Roese]

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

BTW: This was tested on v5.10 and recent v5.16-rc6.

Thanks,
Stefan

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

[Pali Rohár]

Hello! You asked me in another email for comments to this email, so I'm
replying directly to this email...

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.

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. 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). And via AER registers on the Root Port it is
also possible to disable generating MSI(X) interrupts when error is
reported. And IIRC via PCIe Downstream Port Containment there is also
way how to "mask" reporting of error events. But I do not have PCIe
devices with DPC support, so I have not played with it yet. So there are
many places where error event can be stopped. But important is that
kernel AER driver should correctly enable all required bits to start
receiving MSI(X) interrupts for error events.

On other devices I'm seeing following issues... Root Ports are not
compliant to PCIe spec and do not implement error reporting at all. Or
they do not implement those enable/disable bits correctly. Or they do
not implement proper support for extended PCIe config space for Root
Port (AER is in extended space). Or they report error events via custom
proprietary interrupts and not via MSI(X) as required by PCIe spec. This
is the case for (all?) Marvell PCIe controllers and I saw here on
linux-pci list that it applies also for PCIe controllers from some other
vendors. Also drivers for Marvell PCIe controllers requires additional
code to access extended PCIe config space of Root Port (accessing config
space of PCIe devices on the other end of PCIe link is working fine).

So the first suspicious thing is why kernel AER driver is using legacy
shared INTx interrupt as in most cases Root Port would not report any
error event via INTx. And the second thing, try to look into
documentation for used PCIe controller, just in case if vendor
"invented" some proprietary and non-compliant way how to report error /
AER events to OS...

I saw more issues with PCIe controllers as with PCIe switches so in my
opinion issue would be either in controller driver or controller hw
itself. And if you see event status logged in PCIe switch register I
would expect that switch correctly sent PCIe Error message to Root
Complex.

> BTW: This was tested on v5.10 and recent v5.16-rc6.
> 
> Thanks,
> Stefan

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

[Bjorn Helgaas]

On Fri, Jan 07, 2022 at 11:04:58AM +0100, Pali Rohár wrote:
> Hello! You asked me in another email for comments to this email, so I'm
> replying directly to this email...
> 
> 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.

I think the error should also be logged in the Root Port AER
Capability.  And of course the interrupt enable bits in the Root Error
Command register would have to be set.

> > 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

I guess this means whatever INTx the Root Port is using is connected
to IRQ 58?  Can you tell whether that INTx works if a device below the
Root Port uses it?  Or whether it is asserted for PMEs?

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

Where does it say that?  PCIe r5.0, sec 6.2.4.1.2 and 6.2.6, both
mention INTx, and the diagram in 6.2.6 even shows possible
platform-specific System Error signaling.

But I doubt Linux is smart enough to configure this correctly for
INTx.  You could experiment by setting the AER control bits with
setpci.

There was some previous discussion, and it even mentions ZynqMP as a
device that has a dedicated non-MSI mechanism for AER signaling:

  https://lore.kernel.org/linux-pci/1533141889-19962-1-git-send-email-bharat.kumar.gogada@xilinx.com/
  https://lore.kernel.org/all/1464242406-20203-1-git-send-email-po.liu@nxp.com/T/#u

But I don't think it went anywhere.

It seems like maybe this *could* be made to work.  

Bjorn

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

[Pali Rohár]

On Friday 07 January 2022 14:34:15 Bjorn Helgaas wrote:
> On Fri, Jan 07, 2022 at 11:04:58AM +0100, Pali Rohár wrote:
> > Hello! You asked me in another email for comments to this email, so I'm
> > replying directly to this email...
> > 
> > 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.
> 
> I think the error should also be logged in the Root Port AER
> Capability.  And of course the interrupt enable bits in the Root Error
> Command register would have to be set.
> 
> > > 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
> 
> I guess this means whatever INTx the Root Port is using is connected
> to IRQ 58?  Can you tell whether that INTx works if a device below the
> Root Port uses it?  Or whether it is asserted for PMEs?
> 
> > 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.
> 
> Where does it say that?  PCIe r5.0, sec 6.2.4.1.2 and 6.2.6, both
> mention INTx, and the diagram in 6.2.6 even shows possible
> platform-specific System Error signaling.

Kernel AER driver is not available when MSI is not supported:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/pcie/aer.c?h=v5.15#n112
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/pcie/portdrv_core.c?h=v5.15#n224
Originally this was my primary indication that AER is MSI(X)-only.

And my understanding is that AER Root Error Status Register (7.8.4.10)
specifies Advanced Error Interrupt Message Number which indicates which
MSI(X) interrupt is used. And there is no information about INTx if you
enable particular reporting category via AER Root Error Command Register.
That is why I was in impression that AER interrupts are MSI-only.

But now I'm looking at 6.2.4.1.2 section and seems that AER can really
use INTx. So I was wrong here.

But why then kernel AER driver has check that AER is available only when
MSI is enabled? And not available when MSI is disabled?

> But I doubt Linux is smart enough to configure this correctly for
> INTx.  You could experiment by setting the AER control bits with
> setpci.
> 
> There was some previous discussion, and it even mentions ZynqMP as a
> device that has a dedicated non-MSI mechanism for AER signaling:
> 
>   https://lore.kernel.org/linux-pci/1533141889-19962-1-git-send-email-bharat.kumar.gogada@xilinx.com/
>   https://lore.kernel.org/all/1464242406-20203-1-git-send-email-po.liu@nxp.com/T/#u
> 
> But I don't think it went anywhere.
> 
> It seems like maybe this *could* be made to work.  
> 
> Bjorn

So, seems that this is anther PCIe controller which does not report AER
interrupt via standard MSI interrupts but via some vendor-specific way.

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

[Bjorn Helgaas]

On Fri, Jan 07, 2022 at 10:31:06PM +0100, Pali Rohár wrote:
> On Friday 07 January 2022 14:34:15 Bjorn Helgaas wrote:
> > On Fri, Jan 07, 2022 at 11:04:58AM +0100, Pali Rohár wrote:
> > > Hello! You asked me in another email for comments to this email, so I'm
> > > replying directly to this email...
> > > 
> > > 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.
> > 
> > I think the error should also be logged in the Root Port AER
> > Capability.  And of course the interrupt enable bits in the Root Error
> > Command register would have to be set.
> > 
> > > > 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
> > 
> > I guess this means whatever INTx the Root Port is using is connected
> > to IRQ 58?  Can you tell whether that INTx works if a device below the
> > Root Port uses it?  Or whether it is asserted for PMEs?
> > 
> > > 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.
> > 
> > Where does it say that?  PCIe r5.0, sec 6.2.4.1.2 and 6.2.6, both
> > mention INTx, and the diagram in 6.2.6 even shows possible
> > platform-specific System Error signaling.
> 
> Kernel AER driver is not available when MSI is not supported:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/pcie/aer.c?h=v5.15#n112
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/pcie/portdrv_core.c?h=v5.15#n224
> Originally this was my primary indication that AER is MSI(X)-only.

I think that is broken.  Looks like it was added by 3e77a3f7895e
("PCI: Disable AER with pci=nomsi"), which says that with "pci=nomsi",
we see lost interrupts and lockdep inversions.

I don't know any more of the history behind that, but I suspect that
turning off AER in that case just covered up some other Linux issue.

> And my understanding is that AER Root Error Status Register (7.8.4.10)
> specifies Advanced Error Interrupt Message Number which indicates which
> MSI(X) interrupt is used. And there is no information about INTx if you
> enable particular reporting category via AER Root Error Command Register.
> That is why I was in impression that AER interrupts are MSI-only.
> 
> But now I'm looking at 6.2.4.1.2 section and seems that AER can really
> use INTx. So I was wrong here.
> 
> But why then kernel AER driver has check that AER is available only when
> MSI is enabled? And not available when MSI is disabled?

Apart from the issue behind 3e77a3f7895e, I think this is just because
the intersection of platforms that lack MSI and people with enough
interest in AER is small.

The interrupt configuration in portdrv is nasty.  Although it looks
like pcie_init_service_irqs() might now be smart enough to use the
legacy INTx when MSI is not available.

Bjorn

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

[Pali Rohár]

On Friday 07 January 2022 21:13:57 Bjorn Helgaas wrote:
> On Fri, Jan 07, 2022 at 10:31:06PM +0100, Pali Rohár wrote:
> > On Friday 07 January 2022 14:34:15 Bjorn Helgaas wrote:
> > > On Fri, Jan 07, 2022 at 11:04:58AM +0100, Pali Rohár wrote:
> > > > Hello! You asked me in another email for comments to this email, so I'm
> > > > replying directly to this email...
> > > > 
> > > > 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.
> > > 
> > > I think the error should also be logged in the Root Port AER
> > > Capability.  And of course the interrupt enable bits in the Root Error
> > > Command register would have to be set.
> > > 
> > > > > 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
> > > 
> > > I guess this means whatever INTx the Root Port is using is connected
> > > to IRQ 58?  Can you tell whether that INTx works if a device below the
> > > Root Port uses it?  Or whether it is asserted for PMEs?
> > > 
> > > > 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.
> > > 
> > > Where does it say that?  PCIe r5.0, sec 6.2.4.1.2 and 6.2.6, both
> > > mention INTx, and the diagram in 6.2.6 even shows possible
> > > platform-specific System Error signaling.
> > 
> > Kernel AER driver is not available when MSI is not supported:
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/pcie/aer.c?h=v5.15#n112
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/pcie/portdrv_core.c?h=v5.15#n224
> > Originally this was my primary indication that AER is MSI(X)-only.
> 
> I think that is broken.  Looks like it was added by 3e77a3f7895e
> ("PCI: Disable AER with pci=nomsi"), which says that with "pci=nomsi",
> we see lost interrupts and lockdep inversions.
> 
> I don't know any more of the history behind that, but I suspect that
> turning off AER in that case just covered up some other Linux issue.

I have same feeling that something different is broken.

And now it reminds me that I was testing AER interrupts about year ago
and when booted kernel with pci=nomsi then AER kernel driver refused
start and I realized that AER is probably MSI-only. I have checked spec
for AER Root Error Status Register and I due to this I probably come to
the conclusion that AER must be MSI-only...

Is kernel going to enable support for AER when MSI is disabled? Or we
let it in current state forever?

> > And my understanding is that AER Root Error Status Register (7.8.4.10)
> > specifies Advanced Error Interrupt Message Number which indicates which
> > MSI(X) interrupt is used. And there is no information about INTx if you
> > enable particular reporting category via AER Root Error Command Register.
> > That is why I was in impression that AER interrupts are MSI-only.
> > 
> > But now I'm looking at 6.2.4.1.2 section and seems that AER can really
> > use INTx. So I was wrong here.
> > 
> > But why then kernel AER driver has check that AER is available only when
> > MSI is enabled? And not available when MSI is disabled?
> 
> Apart from the issue behind 3e77a3f7895e, I think this is just because
> the intersection of platforms that lack MSI and people with enough
> interest in AER is small.
> 
> The interrupt configuration in portdrv is nasty.  Although it looks
> like pcie_init_service_irqs() might now be smart enough to use the
> legacy INTx when MSI is not available.

In last pci-aardvark patches is added support for emulation of AER
interrupt via virtual INTx and we tested that it is working. portdrv
correctly allocates emulated INTx and when this interrupt is triggered
then kernel AER driver see it and log error into dmesg.

So I guess, it probably could work also with real INTx (if implemented
in HW correctly).

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

[Stefan Roese]

On 1/7/22 21:34, Bjorn Helgaas wrote:
> On Fri, Jan 07, 2022 at 11:04:58AM +0100, Pali Rohár wrote:
>> Hello! You asked me in another email for comments to this email, so I'm
>> replying directly to this email...
>>
>> 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.
> 
> I think the error should also be logged in the Root Port AER
> Capability.  And of course the interrupt enable bits in the Root Error
> Command register would have to be set.

I'm seeing no change at all in the Root Port PCIe device after the
surprise down on one of the PCIe switch downstream ports via
"lspci -vvv".

>>> 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
> 
> I guess this means whatever INTx the Root Port is using is connected
> to IRQ 58?  Can you tell whether that INTx works if a device below the
> Root Port uses it?  Or whether it is asserted for PMEs?

INTx works just fine for "normal" legacy interrupts, e.g. a PCIe
driver requesting a non-MSI interrupt.

>> 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.
> 
> Where does it say that?  PCIe r5.0, sec 6.2.4.1.2 and 6.2.6, both
> mention INTx, and the diagram in 6.2.6 even shows possible
> platform-specific System Error signaling.
> 
> But I doubt Linux is smart enough to configure this correctly for
> INTx.  You could experiment by setting the AER control bits with
> setpci.
> 
> There was some previous discussion, and it even mentions ZynqMP as a
> device that has a dedicated non-MSI mechanism for AER signaling:
> 
>    https://lore.kernel.org/linux-pci/1533141889-19962-1-git-send-email-bharat.kumar.gogada@xilinx.com/
>    https://lore.kernel.org/all/1464242406-20203-1-git-send-email-po.liu@nxp.com/T/#u
> 
> But I don't think it went anywhere.
> 
> It seems like maybe this *could* be made to work.

Thanks Bjorn for the reference. As already mentioned to Pali in the
other mail, Bharat from Xilinx has sent me a link to a newer, updated
patch series to use this "misc" interrupts for AER in the meantime:

https://lore.kernel.org/lkml/1542206878-24587-1-git-send-email-bharat.kumar.gogada@xilinx.com/

AFAICT, this patch series was not really reviewed. At least I can't find
any comments / replies.

I now applied this series (after some merge issues) to v5.16 and re-
tested with this new MISC interrupts for AER. Still no cigar. No
interrupt / AER event upon surprise down on the PEX switch received.

I might have missed something in the setup / configuration though. Is
my understanding correct, that I don't need to "manually" tune the SERR
in the Command register? And is my understanding correct that '0' / '-'
in the AER mask register enables this AER event?

Thanks,
Stefan

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

[Stefan Roese]

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+

> And via AER registers on the Root Port it is
> also possible to disable generating MSI(X) interrupts when error is
> reported.

Here is what the Root Port reports:
         Capabilities: [140 v1] Advanced Error Reporting
                 UESta:  DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- 
UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol-
                 UEMsk:  DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- 
UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol-
                 UESvrt: DLP+ SDES+ TLP- FCP+ CmpltTO- CmpltAbrt- 
UnxCmplt- RxOF+ MalfTLP+ ECRC- UnsupReq- ACSViol-
                 CESta:  RxErr- BadTLP- BadDLLP- Rollover- Timeout- 
AdvNonFatalErr-
                 CEMsk:  RxErr- BadTLP- BadDLLP- Rollover- Timeout- 
AdvNonFatalErr+
                 AERCap: First Error Pointer: 00, ECRCGenCap+ ECRCGenEn- 
ECRCChkCap+ ECRCChkEn-
                         MultHdrRecCap- MultHdrRecEn- TLPPfxPres- HdrLogCap-
                 HeaderLog: 00000000 00000000 00000000 00000000
                 RootCmd: CERptEn+ NFERptEn+ FERptEn+
                 RootSta: CERcvd- MultCERcvd- UERcvd- MultUERcvd-
                          FirstFatal- NonFatalMsg- FatalMsg- IntMsg 0

Mask (..Msk) is set to "-" which is correct for not masking this error
I assume.

> And IIRC via PCIe Downstream Port Containment there is also
> way how to "mask" reporting of error events. But I do not have PCIe
> devices with DPC support, so I have not played with it yet.

Me neither.

> So there are
> many places where error event can be stopped. But important is that
> kernel AER driver should correctly enable all required bits to start
> receiving MSI(X) interrupts for error events.

Agreed.

> On other devices I'm seeing following issues... Root Ports are not
> compliant to PCIe spec and do not implement error reporting at all. Or
> they do not implement those enable/disable bits correctly. Or they do
> not implement proper support for extended PCIe config space for Root
> Port (AER is in extended space). Or they report error events via custom
> proprietary interrupts and not via MSI(X) as required by PCIe spec. This
> is the case for (all?) Marvell PCIe controllers and I saw here on
> linux-pci list that it applies also for PCIe controllers from some other
> vendors. Also drivers for Marvell PCIe controllers requires additional
> code to access extended PCIe config space of Root Port (accessing config
> space of PCIe devices on the other end of PCIe link is working fine).
> 
> So the first suspicious thing is why kernel AER driver is using legacy
> shared INTx interrupt as in most cases Root Port would not report any
> error event via INTx. And the second thing, try to look into
> documentation for used PCIe controller, just in case if vendor
> "invented" some proprietary and non-compliant way how to report error /
> AER events to OS...

At least this seems to be the case. Even though I'm still not receiving
the surprise down error status with additional change. More to this in
the other mail.

> I saw more issues with PCIe controllers as with PCIe switches so in my
> opinion issue would be either in controller driver or controller hw
> itself. And if you see event status logged in PCIe switch register I
> would expect that switch correctly sent PCIe Error message to Root
> Complex.

That is my understanding and best guess right now as well.

Thanks,
Stefan

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

[Stefan Roese]

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:

[  666.630312] nwl-pcie fd0e0000.pcie: Fatal Error in AER Capability
[  666.631390] pcieport 0000:02:02.0: PCIe Bus Error: 
severity=Uncorrected (Fatal), type=Transaction Layer, (Receiver ID)
[  666.632741] pcieport 0000:02:02.0:   device [10b5:8718] error 
status/mask=00000020/00000000
[  666.633817] 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:

[    1.804800] pcieport 0000:00:00.0: get_port_device_capability (225): 
pcie_ports_native=1 host->native_aer=1
[    1.806047] pcieport 0000:00:00.0: AER: 
pci_disable_pcie_error_reporting (246): rc=0
[    1.807294] pcieport 0000:00:00.0: AER: set_device_error_reporting 
(1218): enable=1
[    1.808279] pcieport 0000:00:00.0: AER: 
pci_enable_pcie_error_reporting (233): rc=0
[    1.809250] pci 0000:01:00.0: AER: set_device_error_reporting (1218): 
enable=1
[    1.810196] pci 0000:01:00.0: AER: pci_enable_pcie_error_reporting 
(233): rc=0
[    1.811112] pci 0000:02:01.0: AER: set_device_error_reporting (1218): 
enable=1
[    1.812031] pci 0000:02:01.0: AER: pci_enable_pcie_error_reporting 
(233): rc=0
[    1.812945] pci 0000:02:02.0: AER: set_device_error_reporting (1218): 
enable=1
[    1.813872] pci 0000:02:02.0: AER: pci_enable_pcie_error_reporting 
(233): rc=0
[    1.814788] pci 0000:04:00.0: AER: set_device_error_reporting (1218): 
enable=1
[    1.815702] pci 0000:02:03.0: AER: set_device_error_reporting (1218): 
enable=1
[    1.816620] pci 0000:02:03.0: AER: pci_enable_pcie_error_reporting 
(233): rc=0
[    1.817870] pcieport 0000:01:00.0: get_port_device_capability (225): 
pcie_ports_native=1 host->native_aer=1
[    1.819105] pcieport 0000:01:00.0: AER: 
pci_disable_pcie_error_reporting (246): rc=0
[    1.820991] pcieport 0000:02:01.0: get_port_device_capability (225): 
pcie_ports_native=1 host->native_aer=1
[    1.822279] pcieport 0000:02:01.0: AER: 
pci_disable_pcie_error_reporting (246): rc=0
[    1.824472] pcieport 0000:02:02.0: get_port_device_capability (225): 
pcie_ports_native=1 host->native_aer=1
[    1.825707] pcieport 0000:02:02.0: AER: 
pci_disable_pcie_error_reporting (246): rc=0
[    1.827843] pcieport 0000:02:03.0: get_port_device_capability (225): 
pcie_ports_native=1 host->native_aer=1
[    1.829079] 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?

Thanks,
Stefan

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

[Bjorn Helgaas]

[+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-??]

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

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.

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

Bjorn

[1] https://lore.kernel.org/r/20220111185538.GA152548@bhelgaas
[2] https://lore.kernel.org/r/20220112094251.1271531-1-sr@denx.de

My notes on the call tree in case they're useful for anybody:

  pcie_portdrv_init                   # device_initcall
    pcie_init_services
      pcie_aer_init
        pcie_port_service_register(&aerdriver)
    pci_register_driver(&pcie_portdrive)

  pcie_portdrv_probe
    pcie_port_device_register
      get_port_device_capability
        pci_disable_pcie_error_reporting
          clear CERE NFERE FERE URRE
      pcie_device_init
        device_register               # new service device, available for binding
          ...
            aer_probe                 # driver for new service device

  aer_probe
    if (!PCI_EXP_TYPE_ROOT_PORT)
      return -ENODEV
    aer_enable_rootport
      set_downstream_devices_error_reporting
        pci_walk_bus
          set_device_error_reporting
            pci_enable_pcie_error_reporting
              set CERE NFERE FERE URRE

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

[Stefan Roese]

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

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

[Bjorn Helgaas]

On Thu, Jan 13, 2022 at 08:13:55AM +0100, Stefan Roese wrote:
> On 1/12/22 18:49, Bjorn Helgaas wrote:

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

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

The more common configuration is a Root Port leading directly to an
Endpoint.  In that case, there would be no pcie_portdrv_probe() in the
middle to disable reporting after aer_probe() has enabled it.

The issue you're seeing happens because of the switch in the middle, 
which is becoming more common recently with Thunderbolt.

I poked around on my laptop (Dell 5520 running v5.4):

  00:01.0 Root Port       to [bus 01]          CorrErr-
  01:00.0 NVIDIA GPU                           CorrErr-

  00:1c.0 Root Port       to [bus 02]     AER  CorrErr+
  02:00.0 Intel NIC                       AER  CorrErr-  <-- iwlwifi

  00:1c.1 Root Port       to [bus 03]     AER  CorrErr+
  03:00.0 Realtek card reader             AER  CorrErr-  <-- rtsx_pci

  00:1d.0 Root Port       to [bus 04]     AER  CorrErr+
  04:00.0 NVMe                            AER  CorrErr+

  00:1d.6 Root Port       to [bus 06-3e]  AER  CorrErr+
  06:00.0 Thunderbolt USP to [bus 07-3e]  AER  CorrErr-
  07:00.0 Thunderbolt DSP to [bus 08]     AER  CorrErr-
  ...                                          CorrErr-

Everything in the Thunderbolt hierarchy has reporting disabled,
probably because of the issue you are pointing out.

I can't explain the iwlwifi and rtsx_pci cases.  Both devices have AER
and are directly below a Root Port that also has AER, so I would think
reporting should be enabled.

Bjorn

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

[Stefan Roese]

On 1/13/22 22:32, Bjorn Helgaas wrote:
> On Thu, Jan 13, 2022 at 08:13:55AM +0100, Stefan Roese wrote:
>> On 1/12/22 18:49, Bjorn Helgaas wrote:
> 
>>> 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.
> 
>> 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.
> 
> The more common configuration is a Root Port leading directly to an
> Endpoint.  In that case, there would be no pcie_portdrv_probe() in the
> middle to disable reporting after aer_probe() has enabled it.
> 
> The issue you're seeing happens because of the switch in the middle,
> which is becoming more common recently with Thunderbolt.
> 
> I poked around on my laptop (Dell 5520 running v5.4):
> 
>    00:01.0 Root Port       to [bus 01]          CorrErr-
>    01:00.0 NVIDIA GPU                           CorrErr-
> 
>    00:1c.0 Root Port       to [bus 02]     AER  CorrErr+
>    02:00.0 Intel NIC                       AER  CorrErr-  <-- iwlwifi
> 
>    00:1c.1 Root Port       to [bus 03]     AER  CorrErr+
>    03:00.0 Realtek card reader             AER  CorrErr-  <-- rtsx_pci
> 
>    00:1d.0 Root Port       to [bus 04]     AER  CorrErr+
>    04:00.0 NVMe                            AER  CorrErr+
> 
>    00:1d.6 Root Port       to [bus 06-3e]  AER  CorrErr+
>    06:00.0 Thunderbolt USP to [bus 07-3e]  AER  CorrErr-
>    07:00.0 Thunderbolt DSP to [bus 08]     AER  CorrErr-
>    ...                                          CorrErr-
> 
> Everything in the Thunderbolt hierarchy has reporting disabled,
> probably because of the issue you are pointing out.
> 
> I can't explain the iwlwifi and rtsx_pci cases.  Both devices have AER
> and are directly below a Root Port that also has AER, so I would think
> reporting should be enabled.

This is because AER is enabled for the complete bus via
set_downstream_devices_error_reporting(), which calls
set_device_error_reporting():

static int set_device_error_reporting(struct pci_dev *dev, void *data)
...
	if ((type == PCI_EXP_TYPE_ROOT_PORT) ||
	    (type == PCI_EXP_TYPE_RC_EC) ||
	    (type == PCI_EXP_TYPE_UPSTREAM) ||
	    (type == PCI_EXP_TYPE_DOWNSTREAM)) {
		if (enable)
			pci_enable_pcie_error_reporting(dev);
		else
			pci_disable_pcie_error_reporting(dev);
	}

	if (enable)
		pcie_set_ecrc_checking(dev);

Not sure, why error reporting is not enabled for "normal" PCIe devices
but only pcie_set_ecrc_checking(). This behavior was integrated with the
intial AER support in commit 6c2b374d7485 ("PCI-Express AER
implemetation: AER core and aerdriver") in 2006.

This explains, why you have AER disabled in the DevCtl registers of your
iwlwifi and rtsx_pci devices.

Now you might be asking, why you have AER enabled in the NVMe drive.
This is because the NVMe driver specifically enables AER:

drivers/nvme/host/pci.c:
static int nvme_pci_enable(struct nvme_dev *dev)
{
...
	pci_enable_pcie_error_reporting(pdev);

There are other device drivers, especially ethernet, SCSI etc, which
also specifically call pci_enable_pcie_error_reporting() for their
PCIe devices.

So how to continue with this? Are we "brave enough" to enable AER
for normal PCIe devices as well in set_device_error_reporting()?
This would be a quite big change, as currently all PCIe devices have
AER disabled per default.

And we still have the problem that AER is disabled in all PCIe devices
except for the Root Port. This can be fixed by removing the AER
disabling from get_port_device_capability() as done in the patch I've
sent yesterday to the list.

Another idea that comes to my mind is, to change
pci_enable_pcie_error_reporting() to walk the PCI bus upstream
while enabling the device and enable AER in the DevCtl register in all
upstream PCIe devices (e.g. PCIe switch etc) found on this way up to
the PCIe Root Port. This way, AER will work on PCIe device, where it
is specifically enabled in the device driver. But only there - at
least in cases, where PCIe switches etc are involved.

Did I miss anything?

Thanks,
Stefan

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

[Bjorn Helgaas]

On Fri, Jan 14, 2022 at 07:25:29AM +0100, Stefan Roese wrote:
> On 1/13/22 22:32, Bjorn Helgaas wrote:
> > On Thu, Jan 13, 2022 at 08:13:55AM +0100, Stefan Roese wrote:
> > > On 1/12/22 18:49, Bjorn Helgaas wrote:
> > 
> > > > 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.
> > 
> > > 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.
> > 
> > The more common configuration is a Root Port leading directly to an
> > Endpoint.  In that case, there would be no pcie_portdrv_probe() in the
> > middle to disable reporting after aer_probe() has enabled it.
> > 
> > The issue you're seeing happens because of the switch in the middle,
> > which is becoming more common recently with Thunderbolt.
> > 
> > I poked around on my laptop (Dell 5520 running v5.4):
> > 
> >    00:01.0 Root Port       to [bus 01]          CorrErr-
> >    01:00.0 NVIDIA GPU                           CorrErr-
> > 
> >    00:1c.0 Root Port       to [bus 02]     AER  CorrErr+
> >    02:00.0 Intel NIC                       AER  CorrErr-  <-- iwlwifi
> > 
> >    00:1c.1 Root Port       to [bus 03]     AER  CorrErr+
> >    03:00.0 Realtek card reader             AER  CorrErr-  <-- rtsx_pci
> > 
> >    00:1d.0 Root Port       to [bus 04]     AER  CorrErr+
> >    04:00.0 NVMe                            AER  CorrErr+
> > 
> >    00:1d.6 Root Port       to [bus 06-3e]  AER  CorrErr+
> >    06:00.0 Thunderbolt USP to [bus 07-3e]  AER  CorrErr-
> >    07:00.0 Thunderbolt DSP to [bus 08]     AER  CorrErr-
> >    ...                                          CorrErr-
> > 
> > Everything in the Thunderbolt hierarchy has reporting disabled,
> > probably because of the issue you are pointing out.
> > 
> > I can't explain the iwlwifi and rtsx_pci cases.  Both devices have AER
> > and are directly below a Root Port that also has AER, so I would think
> > reporting should be enabled.
> 
> This is because AER is enabled for the complete bus via
> set_downstream_devices_error_reporting(), which calls
> set_device_error_reporting():
> 
> static int set_device_error_reporting(struct pci_dev *dev, void *data)
> ...
> 	if ((type == PCI_EXP_TYPE_ROOT_PORT) ||
> 	    (type == PCI_EXP_TYPE_RC_EC) ||
> 	    (type == PCI_EXP_TYPE_UPSTREAM) ||
> 	    (type == PCI_EXP_TYPE_DOWNSTREAM)) {
> 		if (enable)
> 			pci_enable_pcie_error_reporting(dev);
> 		else
> 			pci_disable_pcie_error_reporting(dev);
> 	}
> 
> 	if (enable)
> 		pcie_set_ecrc_checking(dev);
> 
> Not sure, why error reporting is not enabled for "normal" PCIe devices
> but only pcie_set_ecrc_checking(). This behavior was integrated with the
> intial AER support in commit 6c2b374d7485 ("PCI-Express AER
> implemetation: AER core and aerdriver") in 2006.
> 
> This explains, why you have AER disabled in the DevCtl registers of your
> iwlwifi and rtsx_pci devices.
> 
> Now you might be asking, why you have AER enabled in the NVMe drive.
> This is because the NVMe driver specifically enables AER:
> 
> drivers/nvme/host/pci.c:
> static int nvme_pci_enable(struct nvme_dev *dev)
> {
> ...
> 	pci_enable_pcie_error_reporting(pdev);
> 
> There are other device drivers, especially ethernet, SCSI etc, which
> also specifically call pci_enable_pcie_error_reporting() for their
> PCIe devices.

Thanks for that investigation!

I think the PCI core should take care this and drivers should not be
in the business of enabling/disabling error reporting unless they have
defects and don't work according to spec.

> So how to continue with this? Are we "brave enough" to enable AER
> for normal PCIe devices as well in set_device_error_reporting()?
> This would be a quite big change, as currently all PCIe devices have
> AER disabled per default.

Good question.  We have "pci=noaer" as a fallback, but it's a poor
experience if users have to discover and use it.  I do think that when
CONFIG_PCIEAER=y, we really should enable AER by default on every
device that supports it.

> And we still have the problem that AER is disabled in all PCIe devices
> except for the Root Port. This can be fixed by removing the AER
> disabling from get_port_device_capability() as done in the patch I've
> sent yesterday to the list.
>
> Another idea that comes to my mind is, to change
> pci_enable_pcie_error_reporting() to walk the PCI bus upstream
> while enabling the device and enable AER in the DevCtl register in all
> upstream PCIe devices (e.g. PCIe switch etc) found on this way up to
> the PCIe Root Port. This way, AER will work on PCIe device, where it
> is specifically enabled in the device driver. But only there - at
> least in cases, where PCIe switches etc are involved.

When we have a choice, I want to get away from pci_walk_bus() (walking
the downstream devices) and also from walking links upstream.
pci_walk_bus() is ugly and needs more care to make sure that whatever
we're doing also happens for future hot-added devices, and walking
upstream is problematic in terms of locking.

Ultimately I think error configuration should be done during the
normal enumeration flow, e.g., somewhere like pci_init_capabilities().

Bjorn

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

[Pali Rohár]

On Friday 14 January 2022 12:30:28 Bjorn Helgaas wrote:
> On Fri, Jan 14, 2022 at 07:25:29AM +0100, Stefan Roese wrote:
> > On 1/13/22 22:32, Bjorn Helgaas wrote:
> > > On Thu, Jan 13, 2022 at 08:13:55AM +0100, Stefan Roese wrote:
> > > > On 1/12/22 18:49, Bjorn Helgaas wrote:
> > > 
> > > > > 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.
> > > 
> > > > 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.
> > > 
> > > The more common configuration is a Root Port leading directly to an
> > > Endpoint.  In that case, there would be no pcie_portdrv_probe() in the
> > > middle to disable reporting after aer_probe() has enabled it.
> > > 
> > > The issue you're seeing happens because of the switch in the middle,
> > > which is becoming more common recently with Thunderbolt.
> > > 
> > > I poked around on my laptop (Dell 5520 running v5.4):
> > > 
> > >    00:01.0 Root Port       to [bus 01]          CorrErr-
> > >    01:00.0 NVIDIA GPU                           CorrErr-
> > > 
> > >    00:1c.0 Root Port       to [bus 02]     AER  CorrErr+
> > >    02:00.0 Intel NIC                       AER  CorrErr-  <-- iwlwifi
> > > 
> > >    00:1c.1 Root Port       to [bus 03]     AER  CorrErr+
> > >    03:00.0 Realtek card reader             AER  CorrErr-  <-- rtsx_pci
> > > 
> > >    00:1d.0 Root Port       to [bus 04]     AER  CorrErr+
> > >    04:00.0 NVMe                            AER  CorrErr+
> > > 
> > >    00:1d.6 Root Port       to [bus 06-3e]  AER  CorrErr+
> > >    06:00.0 Thunderbolt USP to [bus 07-3e]  AER  CorrErr-
> > >    07:00.0 Thunderbolt DSP to [bus 08]     AER  CorrErr-
> > >    ...                                          CorrErr-
> > > 
> > > Everything in the Thunderbolt hierarchy has reporting disabled,
> > > probably because of the issue you are pointing out.
> > > 
> > > I can't explain the iwlwifi and rtsx_pci cases.  Both devices have AER
> > > and are directly below a Root Port that also has AER, so I would think
> > > reporting should be enabled.
> > 
> > This is because AER is enabled for the complete bus via
> > set_downstream_devices_error_reporting(), which calls
> > set_device_error_reporting():
> > 
> > static int set_device_error_reporting(struct pci_dev *dev, void *data)
> > ...
> > 	if ((type == PCI_EXP_TYPE_ROOT_PORT) ||
> > 	    (type == PCI_EXP_TYPE_RC_EC) ||
> > 	    (type == PCI_EXP_TYPE_UPSTREAM) ||
> > 	    (type == PCI_EXP_TYPE_DOWNSTREAM)) {
> > 		if (enable)
> > 			pci_enable_pcie_error_reporting(dev);
> > 		else
> > 			pci_disable_pcie_error_reporting(dev);
> > 	}
> > 
> > 	if (enable)
> > 		pcie_set_ecrc_checking(dev);
> > 
> > Not sure, why error reporting is not enabled for "normal" PCIe devices
> > but only pcie_set_ecrc_checking(). This behavior was integrated with the
> > intial AER support in commit 6c2b374d7485 ("PCI-Express AER
> > implemetation: AER core and aerdriver") in 2006.
> > 
> > This explains, why you have AER disabled in the DevCtl registers of your
> > iwlwifi and rtsx_pci devices.
> > 
> > Now you might be asking, why you have AER enabled in the NVMe drive.
> > This is because the NVMe driver specifically enables AER:
> > 
> > drivers/nvme/host/pci.c:
> > static int nvme_pci_enable(struct nvme_dev *dev)
> > {
> > ...
> > 	pci_enable_pcie_error_reporting(pdev);
> > 
> > There are other device drivers, especially ethernet, SCSI etc, which
> > also specifically call pci_enable_pcie_error_reporting() for their
> > PCIe devices.
> 
> Thanks for that investigation!
> 
> I think the PCI core should take care this and drivers should not be
> in the business of enabling/disabling error reporting unless they have
> defects and don't work according to spec.

I agree too!

> > So how to continue with this? Are we "brave enough" to enable AER
> > for normal PCIe devices as well in set_device_error_reporting()?
> > This would be a quite big change, as currently all PCIe devices have
> > AER disabled per default.
> 
> Good question.  We have "pci=noaer" as a fallback, but it's a poor
> experience if users have to discover and use it.  I do think that when
> CONFIG_PCIEAER=y, we really should enable AER by default on every
> device that supports it.

What about enable it globally for all devices if CONFIG_PCIEAER=y and
pci=noaer is not specified for upcoming -next? It would be enabled in
-next for two release cycles which could be enough to test if there is
any problematic device. And then decide if it goes to -master or not.
pci=noaer is still there...

> > And we still have the problem that AER is disabled in all PCIe devices
> > except for the Root Port. This can be fixed by removing the AER
> > disabling from get_port_device_capability() as done in the patch I've
> > sent yesterday to the list.
> >
> > Another idea that comes to my mind is, to change
> > pci_enable_pcie_error_reporting() to walk the PCI bus upstream
> > while enabling the device and enable AER in the DevCtl register in all
> > upstream PCIe devices (e.g. PCIe switch etc) found on this way up to
> > the PCIe Root Port. This way, AER will work on PCIe device, where it
> > is specifically enabled in the device driver. But only there - at
> > least in cases, where PCIe switches etc are involved.
> 
> When we have a choice, I want to get away from pci_walk_bus() (walking
> the downstream devices) and also from walking links upstream.
> pci_walk_bus() is ugly and needs more care to make sure that whatever
> we're doing also happens for future hot-added devices, and walking
> upstream is problematic in terms of locking.
> 
> Ultimately I think error configuration should be done during the
> normal enumeration flow, e.g., somewhere like pci_init_capabilities().
> 
> Bjorn

I see it in the same way. Function pci_enable_pcie_error_reporting()
probably should not be available for PCIe consumer drivers... At last
because it is not driver related, but rather PCI core related, like
quirks.

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

[Stefan Roese]

On 1/14/22 19:30, Bjorn Helgaas wrote:
> On Fri, Jan 14, 2022 at 07:25:29AM +0100, Stefan Roese wrote:
>> On 1/13/22 22:32, Bjorn Helgaas wrote:
>>> On Thu, Jan 13, 2022 at 08:13:55AM +0100, Stefan Roese wrote:
>>>> On 1/12/22 18:49, Bjorn Helgaas wrote:
>>>
>>>>> 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.
>>>
>>>> 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.
>>>
>>> The more common configuration is a Root Port leading directly to an
>>> Endpoint.  In that case, there would be no pcie_portdrv_probe() in the
>>> middle to disable reporting after aer_probe() has enabled it.
>>>
>>> The issue you're seeing happens because of the switch in the middle,
>>> which is becoming more common recently with Thunderbolt.
>>>
>>> I poked around on my laptop (Dell 5520 running v5.4):
>>>
>>>     00:01.0 Root Port       to [bus 01]          CorrErr-
>>>     01:00.0 NVIDIA GPU                           CorrErr-
>>>
>>>     00:1c.0 Root Port       to [bus 02]     AER  CorrErr+
>>>     02:00.0 Intel NIC                       AER  CorrErr-  <-- iwlwifi
>>>
>>>     00:1c.1 Root Port       to [bus 03]     AER  CorrErr+
>>>     03:00.0 Realtek card reader             AER  CorrErr-  <-- rtsx_pci
>>>
>>>     00:1d.0 Root Port       to [bus 04]     AER  CorrErr+
>>>     04:00.0 NVMe                            AER  CorrErr+
>>>
>>>     00:1d.6 Root Port       to [bus 06-3e]  AER  CorrErr+
>>>     06:00.0 Thunderbolt USP to [bus 07-3e]  AER  CorrErr-
>>>     07:00.0 Thunderbolt DSP to [bus 08]     AER  CorrErr-
>>>     ...                                          CorrErr-
>>>
>>> Everything in the Thunderbolt hierarchy has reporting disabled,
>>> probably because of the issue you are pointing out.
>>>
>>> I can't explain the iwlwifi and rtsx_pci cases.  Both devices have AER
>>> and are directly below a Root Port that also has AER, so I would think
>>> reporting should be enabled.
>>
>> This is because AER is enabled for the complete bus via
>> set_downstream_devices_error_reporting(), which calls
>> set_device_error_reporting():
>>
>> static int set_device_error_reporting(struct pci_dev *dev, void *data)
>> ...
>> 	if ((type == PCI_EXP_TYPE_ROOT_PORT) ||
>> 	    (type == PCI_EXP_TYPE_RC_EC) ||
>> 	    (type == PCI_EXP_TYPE_UPSTREAM) ||
>> 	    (type == PCI_EXP_TYPE_DOWNSTREAM)) {
>> 		if (enable)
>> 			pci_enable_pcie_error_reporting(dev);
>> 		else
>> 			pci_disable_pcie_error_reporting(dev);
>> 	}
>>
>> 	if (enable)
>> 		pcie_set_ecrc_checking(dev);
>>
>> Not sure, why error reporting is not enabled for "normal" PCIe devices
>> but only pcie_set_ecrc_checking(). This behavior was integrated with the
>> intial AER support in commit 6c2b374d7485 ("PCI-Express AER
>> implemetation: AER core and aerdriver") in 2006.
>>
>> This explains, why you have AER disabled in the DevCtl registers of your
>> iwlwifi and rtsx_pci devices.
>>
>> Now you might be asking, why you have AER enabled in the NVMe drive.
>> This is because the NVMe driver specifically enables AER:
>>
>> drivers/nvme/host/pci.c:
>> static int nvme_pci_enable(struct nvme_dev *dev)
>> {
>> ...
>> 	pci_enable_pcie_error_reporting(pdev);
>>
>> There are other device drivers, especially ethernet, SCSI etc, which
>> also specifically call pci_enable_pcie_error_reporting() for their
>> PCIe devices.
> 
> Thanks for that investigation!
> 
> I think the PCI core should take care this and drivers should not be
> in the business of enabling/disabling error reporting unless they have
> defects and don't work according to spec.

Agreed.

>> So how to continue with this? Are we "brave enough" to enable AER
>> for normal PCIe devices as well in set_device_error_reporting()?
>> This would be a quite big change, as currently all PCIe devices have
>> AER disabled per default.
> 
> Good question.  We have "pci=noaer" as a fallback, but it's a poor
> experience if users have to discover and use it.  I do think that when
> CONFIG_PCIEAER=y, we really should enable AER by default on every
> device that supports it.
> 
>> And we still have the problem that AER is disabled in all PCIe devices
>> except for the Root Port. This can be fixed by removing the AER
>> disabling from get_port_device_capability() as done in the patch I've
>> sent yesterday to the list.
>>
>> Another idea that comes to my mind is, to change
>> pci_enable_pcie_error_reporting() to walk the PCI bus upstream
>> while enabling the device and enable AER in the DevCtl register in all
>> upstream PCIe devices (e.g. PCIe switch etc) found on this way up to
>> the PCIe Root Port. This way, AER will work on PCIe device, where it
>> is specifically enabled in the device driver. But only there - at
>> least in cases, where PCIe switches etc are involved.
> 
> When we have a choice, I want to get away from pci_walk_bus() (walking
> the downstream devices) and also from walking links upstream.
> pci_walk_bus() is ugly and needs more care to make sure that whatever
> we're doing also happens for future hot-added devices, and walking
> upstream is problematic in terms of locking.
> 
> Ultimately I think error configuration should be done during the
> normal enumeration flow, e.g., somewhere like pci_init_capabilities().

Okay. Then let's go this way.

Thanks,
Stefan