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
