On Thursday 20 January 2022 08:31:31 Stefan Roese wrote: > 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? You are right! Seems that AER code has lot of candidates for followup fixes/cleanups... > > 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
