Hi Bjorn, > On Dec 2, 2020, at 1:27 PM, Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote: > > On Wed, Dec 02, 2020 at 08:53:54PM +0000, Kelley, Sean V wrote: >>> On Nov 30, 2020, at 4:25 PM, Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote: >>> On Mon, Nov 30, 2020 at 07:54:37PM +0000, Kelley, Sean V wrote: > >>>> - if (pcie_aer_is_native(bridge)) >>>> - pcie_clear_device_status(bridge); >>>> - pci_aer_clear_nonfatal_status(bridge); >>>> >>>> + if (type == PCI_EXP_TYPE_ROOT_PORT || >>>> + type == PCI_EXP_TYPE_DOWNSTREAM || >>>> + type == PCI_EXP_TYPE_RC_EC) { >>>> + if (pcie_aer_is_native(bridge)) >>>> + pcie_clear_device_status(bridge); >>>> + pci_aer_clear_nonfatal_status(bridge); >>>> + } > > Back to this specific hunk, what if we made it this? > > struct pci_host_bridge *host = pci_find_host_bridge(dev->bus); > > if (host->native_aer || pcie_ports_native) { > pcie_clear_device_status(bridge); > pci_aer_clear_nonfatal_status(bridge); > } > > Previously, if "bridge" didn't have an AER Capability, we didn't > pcie_clear_device_status(). In the case of a DPC bridge without AER, > I think we *should* call pcie_clear_device_status(). Agree, I was overlooking DPC here with the AER check. > > Otherwise, I think this should work the same and would be a little > simpler. Looks fine to me. It simplifies it a bit. > >>> It seems like there are basically two devices of interest in >>> pcie_do_recovery(): the endpoint where we have to call the driver >>> error recovery, and the port that generated the interrupt. I wonder >>> if this would make more sense if the caller passed both of them in >>> explicitly instead of having pcie_do_recovery() check the type of >>> "dev" and try to figure things out after the fact. >> >> On this last point I wanted to add that this is a possibility that >> could provide a clearer distinction, especially where actions need >> to be taken or not taken as a part of pcie_do_recovery(), i.e., >> bridge versus dev. In this patch series we have taken steps to >> minimize the need for the distinction by pushing the awareness into >> the driver’s error recovery routine, i.e., dev->rcec. A future >> evolution after this series could lead to both devices of interest >> being passed explicitly for the larger scope EDR/DPC/AER/etc. > > Yeah, not worth doing in *this* series. > > Bjorn Thanks, Sean