On Tue, Aug 21, 2018 at 11:50:54AM -0400, Sinan Kaya wrote: > On 8/21/2018 11:29 AM, Keith Busch wrote: > > > Hotplug driver needs to handle both physical removal as well as intermittent > > > link down issues though. > > Back to your patch you linked to earlier, your proposal is to have > > pciehp wait for DEVSTS.FED before deciding if it needs to handle the > > DLLSC event. That might be a start, but it isn't enough since that > > status isn't set if the downstream device reported ERR_FATAL. I think > > you'd need to check the secondary status register for a Received System > > Error. > > Hmm, good feedback. > > I was trying not to mix AER/DPC with HP but obviously I failed. > I can add a flag to struct pci_dev like aer_pending for AER. > > 1. AER ISR sets aer_pending > 2. AER ISR issues a secondary bus reset > 3. Hotplug driver bails out on aer_pending > 4. AER ISR performs the recovery > > It is slightly more challenging on the DPC front as HW brings down > the link automatically and hotplug driver can observe the link down > event before the DPC ISR. I'll have to go check if DPC interrupt is > pending. > > Let me know if this works out. That sounds like a step in a good direction. I think it's also safe to say the spec requires a slot capable of swapping devices report the PDC slot event, so that should be valid criteria for knowing if a hotplug event occured. There are other issues that we may need to fix. Consider a hierarchy of switches where the root port starts a DPC event and the kernel tries to recover. Meanwhile devices downstream switches lower in the hierarchy have changed. Since recovery doesn't do re-enumeration and the top level downstream port didn't detect a hotplug event, the recovery will "initialize" the wrong device to potentially undefined results. I think we'd need pciehp involved when error recovery rewalks the downstream buses so pciehp may check PDC on capable ports. Implementing 'error_resume' in pciehp to de-enumerate/re-enumerate accordingly might work ... If that happened, pciehp could call pcie_do_fatal_recovery() and all three services could be unified!
