Hi, I've been testing this out with various endpoints (both upstream and not...), and I have a question that intersects with this area:
On Tue, Jul 15, 2025 at 07:51:05PM +0530, Manivannan Sadhasivam via B4 Relay wrote: > From: Manivannan Sadhasivam <m...@kernel.org> > > The PCI link, when down, needs to be recovered to bring it back. But on > some platforms, that cannot be done in a generic way as link recovery > procedure is platform specific. So add a new API > pci_host_handle_link_down() that could be called by the host bridge drivers > for a specific Root Port when the link goes down. > > The API accepts the 'pci_dev' corresponding to the Root Port which observed > the link down event. If CONFIG_PCIEAER is enabled, the API calls > pcie_do_recovery() function with 'pci_channel_io_frozen' as the state. This > will result in the execution of the AER Fatal error handling code. Since > the link down recovery is pretty much the same as AER Fatal error handling, > pcie_do_recovery() helper is reused here. First, the AER error_detected() > callback will be triggered for the bridge and then for the downstream > devices. I've been trying to understand what exactly the .error_detected() involvement should be here (and what it actually does, despite the docs), and especially around its return codes. Specifically, I'm trying to see what's supposed to happen with PCI_ERS_RESULT_CAN_RECOVER. I see that for pci_channel_io_frozen, almost all endpoint drivers return PCI_ERS_RESULT_NEED_RESET, but if drivers actually return PCI_ERS_RESULT_CAN_RECOVER, it's unclear what should happen. Today, we don't actually respect it; pcie_do_recovery() just calls reset_subordinates() (pci_host_reset_root_port()) unconditionally. The only thing that return code affects is whether we call report_mmio_enabled() vs report_slot_reset() afterward. This seems odd. It also doesn't totally match the docs: https://docs.kernel.org/PCI/pcieaer-howto.html#non-correctable-non-fatal-and-fatal-errors https://docs.kernel.org/PCI/pci-error-recovery.html e.g., "PCI_ERS_RESULT_CAN_RECOVER Driver returns this if it thinks it might be able to recover the HW by just banging IOs or if it wants to be given a chance to extract some diagnostic information (see mmio_enable, below)." I've seen drivers that think they want to handle stuff on their own -- for example, if they have a handle to an external PMIC, they may try to reset things that way -- and so they return PCI_ERS_RESULT_CAN_RECOVER even for io_frozen. I'm not convinced that's a great idea, but I'm also not sure what to say about the docs. On the flip side: it's not clear PCI_ERS_RESULT_NEED_RESET+pci_channel_io_normal works as documented either. An endpoint might think it's requesting a slot reset, but pcie_do_recovery() will ignore that and skip reset_subordinates() (pci_host_reset_root_port()). All in all, the docs sound like endpoints _should_ have control over whether we exercise a full port/slot reset for all types of errors. But in practice, we do not actually give it that control. i.e., your commit message is correct, and the docs are not. I have half a mind to suggest the appended change, so the behavior matches (some of) the docs a little better [1]. Brian > Finally, pci_host_reset_root_port() will be called for the Root > Port, which will reset the Root Port using 'reset_root_port' callback to > recover the link. Once that's done, resume message will be broadcasted to > the bridge and the downstream devices, indicating successful link recovery. > > But if CONFIG_PCIEAER is not enabled in the kernel, only > pci_host_reset_root_port() API will be called, which will in turn call > pci_bus_error_reset() to just reset the Root Port as there is no way we > could inform the drivers about link recovery. > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasi...@linaro.org> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasi...@oss.qualcomm.com> [1] --- a/drivers/pci/pcie/err.c +++ b/drivers/pci/pcie/err.c @@ -219,13 +219,10 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev, pci_dbg(bridge, "broadcast error_detected message\n"); if (state == pci_channel_io_frozen) { pci_walk_bridge(bridge, report_frozen_detected, &status); - if (reset_subordinates(bridge) != PCI_ERS_RESULT_RECOVERED) { - pci_warn(bridge, "subordinate device reset failed\n"); - goto failed; - } } else { pci_walk_bridge(bridge, report_normal_detected, &status); } + pci_dbg(bridge, "error_detected result: %d\n", status); if (status == PCI_ERS_RESULT_CAN_RECOVER) { status = PCI_ERS_RESULT_RECOVERED; @@ -234,6 +231,11 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev, } if (status == PCI_ERS_RESULT_NEED_RESET) { + if (reset_subordinates(bridge) != PCI_ERS_RESULT_RECOVERED) { + pci_warn(bridge, "subordinate device reset failed\n"); + goto failed; + } + status = PCI_ERS_RESULT_RECOVERED; pci_dbg(bridge, "broadcast slot_reset message\n"); pci_walk_bridge(bridge, report_slot_reset, &status);
