Re: [PATCH v3 2/2] PCI/AER: Enable AER on all PCIe devices supporting it

[Stefan Roese <sr@denx.de>]

On 1/19/22 11:37, Pali Rohár wrote:
> On Wednesday 19 January 2022 10:22:00 Stefan Roese wrote:
>> With this change, AER is now enabled on all PCIe devices, also when the
>> PCIe device is hot-plugged.
>>
>> Please note that this change is quite invasive, as with this patch
>> applied, AER now will be enabled in the Device Control registers of all
>> available PCIe Endpoints, which currently is not the case.
>>
>> When "pci=noaer" is selected, AER stays disabled of course.
> 
> Hello Stefan! I was thinking more about this change and I'm not sure
> what happens if AER-capable PCIe device is hotplugged into some PCIe
> switch connected in the PCIe hierarchy where Root Port is not
> AER-capable (e.g. current linux implementation of pci-aardvark.c and
> pci-mvebu.c). My feeling is that in this case AER should not be enabled
> as there is nobody who can deliver AER interrupt to the OS. But I really
> do not know what is supposed from kernel AER driver, so lets wait for
> Bjorn reply.

But what happens right now, when a device driver like the NVMe driver
calls pci_enable_pcie_error_reporting() ? There is also no checking,
if the connected Root Port or some switch / bridge in-between supports
AER or not. IIUTC, this is identical to what this patch here does.
Enable AER in the device and if the upstream infrastructure does not
support AER, then the AER event will just not be received by the
Kernel. Which is most likely not worse than not enabling AER at all
on this device. Or am I missing something?

> And when you opened this issue with hotplugging, another thing for
> followup changes in future is calling pcie_set_ecrc_checking() function
> to align ECRC state of newly hotplugged device with "pci=ecrc=..."
> cmdline option. As currently it is done only at that function
> set_device_error_reporting().

Agreed, this is another area to look into. Not sure if it's okay to
address this, once this patch-set has been accepted (if it will be).

Thanks,
Stefan