On Thu, Oct 18, 2018 at 08:11:35PM -0500, Bjorn Helgaas wrote: > On Thu, Oct 18, 2018 at 05:03:13PM -0600, Keith Busch wrote: > > On Thu, Oct 18, 2018 at 03:53:58PM -0500, Bjorn Helgaas wrote: > > > Change the AER service driver so it binds to *all* PCIe Ports, > > > including Switch Upstream and Downstream Ports. Enable AER error > > > reporting for all these Ports, but not for any children. > > > > I'm looking at this again and think enabling/disabling error > > reporting for ports is the responsibility of the port driver, not > > the AER service. > > That's an interesting idea. Can you expand on this a little more? > Why is it the responsibility of the port driver? > > Do you think pci_enable_pcie_error_reporting() shouldn't be part of > the AER service because it updates the Device Control register, which > is in the PCIe Capability, not the AER Capability? > > What about pci_aer_clear_device_status(), which clears Device Status, > which is also in the PCIe Capability? I was comparing how other end device driver's enable this, and they all call pci_enable_pcie_error_reporting() somewhere along their pci_driver->probe path. With that in mind, are ports special compared to end devices for this particular feature? > > The following should do the same as this patch, but without making > > AER driver handle non-root ports. The report enabling/disabling > > functions are already stubbed for '!CONFIG_PCIE_AER' and have checks > > for aer_cap and firmware first. > > If we thought we should enable error reporting *always*, regardless of > whether the AER service is enabled, this would make perfect sense to > me, and I might suggest doing it in an even more generic place like > pci_configure_device() or pci_init_capabilities(). There are unfortunately still pci_driver instances that don't implement the err_handler callbacks, and may cause problems if we enable error reporting in the device when its driver isn't capable of reacting to them. If it wasn't for that, I think it would make more sense to move this responsibility from drivers to the pci core. > But that doesn't seem like where you're headed. It seems like you > still only want error reporting enabled when CONFIG_PCIEAR=y. If > that's the case, it seems like doing it in portdrv only obfuscates the > connection with AER. When CONFIG_PCIEAER is unset, the portdrv code > *looks* like it's doing something but it's really not because of the > #ifdef magic. Right, but that's no different than every other Linux pci_driver. The component that provides the pci_driver.err_handler should be responsible for requesting to enable device error reporting, and that's provided by the port driver, not AER.
