From: Manivannan Sadhasivam <mani@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. 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.sadhasivam@linaro.org>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
drivers/pci/controller/pci-host-common.c | 33 ++++++++++++++++++++++++++++++++
drivers/pci/controller/pci-host-common.h | 1 +
drivers/pci/pci.c | 1 +
drivers/pci/pcie/err.c | 1 +
4 files changed, 36 insertions(+)
diff --git a/drivers/pci/controller/pci-host-common.c b/drivers/pci/controller/pci-host-common.c
index b0992325dd65f0da8e216ec8a2153af365225d1d..51eacb6cb57443338e995f17afd3b2564bbd1f83 100644
--- a/drivers/pci/controller/pci-host-common.c
+++ b/drivers/pci/controller/pci-host-common.c
@@ -12,9 +12,11 @@
#include <linux/of.h>
#include <linux/of_address.h>
#include <linux/of_pci.h>
+#include <linux/pci.h>
#include <linux/pci-ecam.h>
#include <linux/platform_device.h>
+#include "../pci.h"
#include "pci-host-common.h"
static void gen_pci_unmap_cfg(void *ptr)
@@ -104,5 +106,36 @@ void pci_host_common_remove(struct platform_device *pdev)
}
EXPORT_SYMBOL_GPL(pci_host_common_remove);
+static pci_ers_result_t pci_host_reset_root_port(struct pci_dev *dev)
+{
+ int ret;
+
+ ret = pci_bus_error_reset(dev);
+ if (ret) {
+ pci_err(dev, "Failed to reset Root Port: %d\n", ret);
+ return PCI_ERS_RESULT_DISCONNECT;
+ }
+
+ pci_info(dev, "Root Port has been reset\n");
+
+ return PCI_ERS_RESULT_RECOVERED;
+}
+
+static void pci_host_recover_root_port(struct pci_dev *port)
+{
+#if IS_ENABLED(CONFIG_PCIEAER)
+ pcie_do_recovery(port, pci_channel_io_frozen, pci_host_reset_root_port);
+#else
+ pci_host_reset_root_port(port);
+#endif
+}
+
+void pci_host_handle_link_down(struct pci_dev *port)
+{
+ pci_info(port, "Recovering Root Port due to Link Down\n");
+ pci_host_recover_root_port(port);
+}
+EXPORT_SYMBOL_GPL(pci_host_handle_link_down);
+
MODULE_DESCRIPTION("Common library for PCI host controller drivers");
MODULE_LICENSE("GPL v2");
diff --git a/drivers/pci/controller/pci-host-common.h b/drivers/pci/controller/pci-host-common.h
index 65bd9e032353827221a6af59858c46fdbe5916bf..cb0a07c8773ec87838164e994b34a62d2c8118be 100644
--- a/drivers/pci/controller/pci-host-common.h
+++ b/drivers/pci/controller/pci-host-common.h
@@ -16,5 +16,6 @@ int pci_host_common_probe(struct platform_device *pdev);
int pci_host_common_init(struct platform_device *pdev,
const struct pci_ecam_ops *ops);
void pci_host_common_remove(struct platform_device *pdev);
+void pci_host_handle_link_down(struct pci_dev *port);
#endif
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index b29264aa2be33b18a58b3b3db1d1fb0f6483e5e8..39310422634a9551efc8aded565b7cc30f4989d0 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5768,6 +5768,7 @@ int pci_bus_error_reset(struct pci_dev *bridge)
mutex_unlock(&pci_slot_mutex);
return pci_bus_reset(bridge->subordinate, PCI_RESET_DO_RESET);
}
+EXPORT_SYMBOL_GPL(pci_bus_error_reset);
/**
* pci_probe_reset_bus - probe whether a PCI bus can be reset
diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
index b834fc0d705938540d3d7d3d8739770c09fe7cf1..3e3084bb7cb7fa06b526e6fab60e77927aba0ad0 100644
--- a/drivers/pci/pcie/err.c
+++ b/drivers/pci/pcie/err.c
@@ -270,3 +270,4 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
return status;
}
+EXPORT_SYMBOL_GPL(pcie_do_recovery);
--
2.45.2Hi,
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 <mani@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.sadhasivam@linaro.org>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@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);
On Thu, Aug 28, 2025 at 01:25:12PM -0700, Brian Norris wrote: > 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]. A change similar to the one you're proposing is already queued on the pci/aer topic branch for v6.18: https://git.kernel.org/pci/pci/c/d0a2dee7d458 Here's the corresponding cover letter: https://lore.kernel.org/r/cover.1755008151.git.lukas@wunner.de There was a discussion why I didn't take the exact same approach you're proposing, but only a similar one: https://lore.kernel.org/r/aJ2uE6v46Zib30Jh@wunner.de https://lore.kernel.org/r/aKHWf3L0NCl_CET5@wunner.de > 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. In the series queued on pci/aer, I've only allowed drivers to opt in to a reset on Non-Fatal Errors. I didn't dare also letting them opt out of a reset on Fatal Errors. These changes of behavior are always risky, so it seemed prudent to not introduce too many changes at once. There was no urgent need to also change behavior for Fatal Errors for the use case at hand (the xe graphics driver). I went through all drivers with pci_error_handlers to avoid breaking any of them. It's very tedious work, takes weeks. It would be necessary to do that again when changing behavior for Fatal Errors. pcieaer-howto.rst justifies the unconditional reset on Fatal Errors by saying that the link is unreliable and that a reset is thus required. On the other hand, pci-error-recovery.rst (which is a few months older than pcieaer-howto.rst) says in section "STEP 3: Link Reset": "This is a PCIe specific step and is done whenever a fatal error has been detected" I'm wondering if the authors of pcieaer-howto.rst took that at face value and thought they'd *have* to reset the link on Fatal Errors. Looking through the Fatal Errors in PCIe r7.0 sec 6.2.7, I think a reset is justified for some of them, but optional for others. Which leads me to believe that the AER driver should actually enforce a reset only for certain Fatal Errors, not all of them. So this seems like something worth revisiting in the future. > 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. Indeed the documentation is no longer in sync with the code. I've just submitted a series to rectify that and cc'ed you: https://lore.kernel.org/r/cover.1756451884.git.lukas@wunner.de Thanks, Lukas
Hi Lukas, On Fri, Aug 29, 2025 at 10:35:20AM +0200, Lukas Wunner wrote: > On Thu, Aug 28, 2025 at 01:25:12PM -0700, Brian Norris wrote: > > 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]. > > A change similar to the one you're proposing is already queued on the > pci/aer topic branch for v6.18: > > https://git.kernel.org/pci/pci/c/d0a2dee7d458 Wow, nice coincidence. It's a reminder I should work off the maintainer / -next branch, instead of just mainline... > Here's the corresponding cover letter: > > https://lore.kernel.org/r/cover.1755008151.git.lukas@wunner.de > > There was a discussion why I didn't take the exact same approach you're > proposing, but only a similar one: > > https://lore.kernel.org/r/aJ2uE6v46Zib30Jh@wunner.de > https://lore.kernel.org/r/aKHWf3L0NCl_CET5@wunner.de Wow, that's a ton of great background and explanation. Thanks! > > 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. > > In the series queued on pci/aer, I've only allowed drivers to opt in > to a reset on Non-Fatal Errors. I didn't dare also letting them opt > out of a reset on Fatal Errors. Right, I can see where the latter is risky. Frankly, while I have endpoint drivers suggesting they should be able to do this, I'm not sure that's a great idea. Or at least, I can see how it would potentially break other clients, as you explain. > These changes of behavior are always risky, so it seemed prudent to not > introduce too many changes at once. There was no urgent need to also > change behavior for Fatal Errors for the use case at hand (the xe graphics > driver). I went through all drivers with pci_error_handlers to avoid > breaking any of them. It's very tedious work, takes weeks. It would > be necessary to do that again when changing behavior for Fatal Errors. > > pcieaer-howto.rst justifies the unconditional reset on Fatal Errors by > saying that the link is unreliable and that a reset is thus required. > > On the other hand, pci-error-recovery.rst (which is a few months older > than pcieaer-howto.rst) says in section "STEP 3: Link Reset": > "This is a PCIe specific step and is done whenever a fatal error has been > detected" > > I'm wondering if the authors of pcieaer-howto.rst took that at face value > and thought they'd *have* to reset the link on Fatal Errors. > > Looking through the Fatal Errors in PCIe r7.0 sec 6.2.7, I think a reset > is justified for some of them, but optional for others. Which leads me > to believe that the AER driver should actually enforce a reset only for > certain Fatal Errors, not all of them. So this seems like something > worth revisiting in the future. Hmm, possibly. I haven't looked so closely at the details on all Fatal Errors, but I may have a look eventually. > > 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. > > Indeed the documentation is no longer in sync with the code. I've just > submitted a series to rectify that and cc'ed you: > > https://lore.kernel.org/r/cover.1756451884.git.lukas@wunner.de Thanks! I'll try to take a pass at reviewing, but it may not be prompt. Thanks again for all the info and work here. Brian
On Tue, Jul 15, 2025 at 07:51:05PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> From: Manivannan Sadhasivam <mani@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. 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.sadhasivam@linaro.org>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
> ---
> drivers/pci/controller/pci-host-common.c | 33 ++++++++++++++++++++++++++++++++
> drivers/pci/controller/pci-host-common.h | 1 +
> drivers/pci/pci.c | 1 +
> drivers/pci/pcie/err.c | 1 +
> 4 files changed, 36 insertions(+)
>
> diff --git a/drivers/pci/controller/pci-host-common.c b/drivers/pci/controller/pci-host-common.c
> index b0992325dd65f0da8e216ec8a2153af365225d1d..51eacb6cb57443338e995f17afd3b2564bbd1f83 100644
> --- a/drivers/pci/controller/pci-host-common.c
> +++ b/drivers/pci/controller/pci-host-common.c
> @@ -12,9 +12,11 @@
> #include <linux/of.h>
> #include <linux/of_address.h>
> #include <linux/of_pci.h>
> +#include <linux/pci.h>
> #include <linux/pci-ecam.h>
> #include <linux/platform_device.h>
>
> +#include "../pci.h"
> #include "pci-host-common.h"
>
> static void gen_pci_unmap_cfg(void *ptr)
> @@ -104,5 +106,36 @@ void pci_host_common_remove(struct platform_device *pdev)
> }
> EXPORT_SYMBOL_GPL(pci_host_common_remove);
>
> +static pci_ers_result_t pci_host_reset_root_port(struct pci_dev *dev)
> +{
> + int ret;
> +
> + ret = pci_bus_error_reset(dev);
> + if (ret) {
> + pci_err(dev, "Failed to reset Root Port: %d\n", ret);
> + return PCI_ERS_RESULT_DISCONNECT;
> + }
> +
> + pci_info(dev, "Root Port has been reset\n");
> +
> + return PCI_ERS_RESULT_RECOVERED;
> +}
> +
> +static void pci_host_recover_root_port(struct pci_dev *port)
> +{
> +#if IS_ENABLED(CONFIG_PCIEAER)
> + pcie_do_recovery(port, pci_channel_io_frozen, pci_host_reset_root_port);
> +#else
> + pci_host_reset_root_port(port);
> +#endif
> +}
> +
> +void pci_host_handle_link_down(struct pci_dev *port)
> +{
> + pci_info(port, "Recovering Root Port due to Link Down\n");
> + pci_host_recover_root_port(port);
> +}
> +EXPORT_SYMBOL_GPL(pci_host_handle_link_down);
> +
> MODULE_DESCRIPTION("Common library for PCI host controller drivers");
> MODULE_LICENSE("GPL v2");
> diff --git a/drivers/pci/controller/pci-host-common.h b/drivers/pci/controller/pci-host-common.h
> index 65bd9e032353827221a6af59858c46fdbe5916bf..cb0a07c8773ec87838164e994b34a62d2c8118be 100644
> --- a/drivers/pci/controller/pci-host-common.h
> +++ b/drivers/pci/controller/pci-host-common.h
> @@ -16,5 +16,6 @@ int pci_host_common_probe(struct platform_device *pdev);
> int pci_host_common_init(struct platform_device *pdev,
> const struct pci_ecam_ops *ops);
> void pci_host_common_remove(struct platform_device *pdev);
> +void pci_host_handle_link_down(struct pci_dev *port);
>
> #endif
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index b29264aa2be33b18a58b3b3db1d1fb0f6483e5e8..39310422634a9551efc8aded565b7cc30f4989d0 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5768,6 +5768,7 @@ int pci_bus_error_reset(struct pci_dev *bridge)
> mutex_unlock(&pci_slot_mutex);
> return pci_bus_reset(bridge->subordinate, PCI_RESET_DO_RESET);
> }
> +EXPORT_SYMBOL_GPL(pci_bus_error_reset);
>
> /**
> * pci_probe_reset_bus - probe whether a PCI bus can be reset
> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
> index b834fc0d705938540d3d7d3d8739770c09fe7cf1..3e3084bb7cb7fa06b526e6fab60e77927aba0ad0 100644
> --- a/drivers/pci/pcie/err.c
> +++ b/drivers/pci/pcie/err.c
> @@ -270,3 +270,4 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
>
> return status;
> }
> +EXPORT_SYMBOL_GPL(pcie_do_recovery);
>
> --
> 2.45.2
>
>
© 2016 - 2025 Red Hat, Inc.