Since commit 7b42d97e99d3 ("PCI/ERR: Always report current recovery
status for udev") AER uses the result of error_detected() as parameter
to pci_uevent_ers(). As pci_uevent_ers() however does not handle
PCI_ERS_RESULT_NEED_RESET this results in a missing uevent for the
beginning of recovery if drivers request a reset. Fix this by treating
PCI_ERS_RESULT_NEED_RESET as beginning recovery.
Cc: stable@vger.kernel.org
Fixes: 7b42d97e99d3 ("PCI/ERR: Always report current recovery status for udev")
Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
---
drivers/pci/pci-driver.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 67db34fd10ee7101baeeaae1bb9bec3b13e2fdeb..94ba6938b7c6271b557cc7f17ffb89631d83827e 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -1592,6 +1592,7 @@ void pci_uevent_ers(struct pci_dev *pdev, enum pci_ers_result err_type)
switch (err_type) {
case PCI_ERS_RESULT_NONE:
case PCI_ERS_RESULT_CAN_RECOVER:
+ case PCI_ERS_RESULT_NEED_RESET:
envp[idx++] = "ERROR_EVENT=BEGIN_RECOVERY";
envp[idx++] = "DEVICE_ONLINE=0";
break;
--
2.48.1On Wed, Jul 30, 2025 at 01:20:57PM +0200, Niklas Schnelle wrote:
> Since commit 7b42d97e99d3 ("PCI/ERR: Always report current recovery
> status for udev") AER uses the result of error_detected() as parameter
> to pci_uevent_ers(). As pci_uevent_ers() however does not handle
> PCI_ERS_RESULT_NEED_RESET this results in a missing uevent for the
> beginning of recovery if drivers request a reset. Fix this by treating
> PCI_ERS_RESULT_NEED_RESET as beginning recovery.
>
> Cc: stable@vger.kernel.org
> Fixes: 7b42d97e99d3 ("PCI/ERR: Always report current recovery status for udev")
> Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
Reviewed-by: Lukas Wunner <lukas@wunner.de>
On Wed, Jul 30, 2025 at 01:20:57PM +0200, Niklas Schnelle wrote:
> Since commit 7b42d97e99d3 ("PCI/ERR: Always report current recovery
> status for udev") AER uses the result of error_detected() as parameter
> to pci_uevent_ers(). As pci_uevent_ers() however does not handle
> PCI_ERS_RESULT_NEED_RESET this results in a missing uevent for the
> beginning of recovery if drivers request a reset. Fix this by treating
> PCI_ERS_RESULT_NEED_RESET as beginning recovery.
[...]
> +++ b/drivers/pci/pci-driver.c
> @@ -1592,6 +1592,7 @@ void pci_uevent_ers(struct pci_dev *pdev, enum pci_ers_result err_type)
> switch (err_type) {
> case PCI_ERS_RESULT_NONE:
> case PCI_ERS_RESULT_CAN_RECOVER:
> + case PCI_ERS_RESULT_NEED_RESET:
> envp[idx++] = "ERROR_EVENT=BEGIN_RECOVERY";
> envp[idx++] = "DEVICE_ONLINE=0";
> break;
I note that PCI_ERS_RESULT_NO_AER_DRIVER is also missing in that
switch/case statement. I guess for the patch to be complete,
it needs to be added to the PCI_ERS_RESULT_DISCONNECT case.
Do you agree?
If you do and respin the patch with that change, feel free to add
my Reviewed-by.
Since you're an IBMer and EEH is maintained by IBM, I'm wondering
if it would be possible to amend EEH to report "rc" instead of
PCI_ERS_RESULT_NONE in eeh_report_error()? There are multiple
deviations between AER and EEH, this is one of them. It would
be good to move towards a more consistent recovery process across
platforms.
Thanks,
Lukas
On Wed, Jul 30, 2025 at 10:01:50PM +0200, Lukas Wunner wrote:
> On Wed, Jul 30, 2025 at 01:20:57PM +0200, Niklas Schnelle wrote:
> > Since commit 7b42d97e99d3 ("PCI/ERR: Always report current recovery
> > status for udev") AER uses the result of error_detected() as parameter
> > to pci_uevent_ers(). As pci_uevent_ers() however does not handle
> > PCI_ERS_RESULT_NEED_RESET this results in a missing uevent for the
> > beginning of recovery if drivers request a reset. Fix this by treating
> > PCI_ERS_RESULT_NEED_RESET as beginning recovery.
> [...]
> > +++ b/drivers/pci/pci-driver.c
> > @@ -1592,6 +1592,7 @@ void pci_uevent_ers(struct pci_dev *pdev, enum pci_ers_result err_type)
> > switch (err_type) {
> > case PCI_ERS_RESULT_NONE:
> > case PCI_ERS_RESULT_CAN_RECOVER:
> > + case PCI_ERS_RESULT_NEED_RESET:
> > envp[idx++] = "ERROR_EVENT=BEGIN_RECOVERY";
> > envp[idx++] = "DEVICE_ONLINE=0";
> > break;
>
> I note that PCI_ERS_RESULT_NO_AER_DRIVER is also missing in that
> switch/case statement. I guess for the patch to be complete,
> it needs to be added to the PCI_ERS_RESULT_DISCONNECT case.
> Do you agree?
I realize now there's a bigger problem here: In pcie_do_recovery(),
when control reaches the "failed:" label, a uevent is only signaled
for the *bridge*. Shouldn't a uevent instead be signaled for every
device *below* the bridge? (And possibly the bridge itself if it was
the device reporting the error.)
In that case you don't need to add PCI_ERS_RESULT_NO_AER_DRIVER to
the switch/case statement because we wouldn't want to have multiple
uevents reporting disconnect, so the one emitted below the "failed:"
label would be sufficient.
Right now we may report BEGIN_RECOVERY to user space, but we fail to
later on signal FAILED_RECOVERY (unless I'm missing something).
This all looks so broken that I'm starting to wonder if there's any
user space application at all that takes advantage of these uevents?
Thanks,
Lukas
On Wed, Jul 30, 2025 at 10:24:07PM +0200, Lukas Wunner wrote:
> On Wed, Jul 30, 2025 at 10:01:50PM +0200, Lukas Wunner wrote:
> > On Wed, Jul 30, 2025 at 01:20:57PM +0200, Niklas Schnelle wrote:
> > > Since commit 7b42d97e99d3 ("PCI/ERR: Always report current recovery
> > > status for udev") AER uses the result of error_detected() as parameter
> > > to pci_uevent_ers(). As pci_uevent_ers() however does not handle
> > > PCI_ERS_RESULT_NEED_RESET this results in a missing uevent for the
> > > beginning of recovery if drivers request a reset. Fix this by treating
> > > PCI_ERS_RESULT_NEED_RESET as beginning recovery.
> > [...]
> > > +++ b/drivers/pci/pci-driver.c
> > > @@ -1592,6 +1592,7 @@ void pci_uevent_ers(struct pci_dev *pdev, enum pci_ers_result err_type)
> > > switch (err_type) {
> > > case PCI_ERS_RESULT_NONE:
> > > case PCI_ERS_RESULT_CAN_RECOVER:
> > > + case PCI_ERS_RESULT_NEED_RESET:
> > > envp[idx++] = "ERROR_EVENT=BEGIN_RECOVERY";
> > > envp[idx++] = "DEVICE_ONLINE=0";
> > > break;
> >
> > I note that PCI_ERS_RESULT_NO_AER_DRIVER is also missing in that
> > switch/case statement. I guess for the patch to be complete,
> > it needs to be added to the PCI_ERS_RESULT_DISCONNECT case.
> > Do you agree?
>
> I realize now there's a bigger problem here: In pcie_do_recovery(),
> when control reaches the "failed:" label, a uevent is only signaled
> for the *bridge*. Shouldn't a uevent instead be signaled for every
> device *below* the bridge? (And possibly the bridge itself if it was
> the device reporting the error.)
The small patch below should resolve this issue.
Please let me know what you think.
> In that case you don't need to add PCI_ERS_RESULT_NO_AER_DRIVER to
> the switch/case statement because we wouldn't want to have multiple
> uevents reporting disconnect, so the one emitted below the "failed:"
> label would be sufficient.
I'll send a separate Reviewed-by for your original patch as the small
patch below should resolve my concern about PCI_ERS_RESULT_NO_AER_DRIVER.
> This all looks so broken that I'm starting to wonder if there's any
> user space application at all that takes advantage of these uevents?
I'd still be interested to know which user space application you're
using to track these uevents?
Thanks,
Lukas
-- >8 --
diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
index e795e5ae..3a95aa2 100644
--- a/drivers/pci/pcie/err.c
+++ b/drivers/pci/pcie/err.c
@@ -165,6 +165,12 @@ static int report_resume(struct pci_dev *dev, void *data)
return 0;
}
+static int report_disconnect(struct pci_dev *dev, void *data)
+{
+ pci_uevent_ers(dev, PCI_ERS_RESULT_DISCONNECT);
+ return 0;
+}
+
/**
* pci_walk_bridge - walk bridges potentially AER affected
* @bridge: bridge which may be a Port, an RCEC, or an RCiEP
@@ -272,7 +278,7 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
failed:
pci_walk_bridge(bridge, pci_pm_runtime_put, NULL);
- pci_uevent_ers(bridge, PCI_ERS_RESULT_DISCONNECT);
+ pci_walk_bridge(bridge, report_disconnect, NULL);
pci_info(bridge, "device recovery failed\n");
On Thu, 2025-07-31 at 15:01 +0200, Lukas Wunner wrote:
> On Wed, Jul 30, 2025 at 10:24:07PM +0200, Lukas Wunner wrote:
> > On Wed, Jul 30, 2025 at 10:01:50PM +0200, Lukas Wunner wrote:
> > > On Wed, Jul 30, 2025 at 01:20:57PM +0200, Niklas Schnelle wrote:
> > > > Since commit 7b42d97e99d3 ("PCI/ERR: Always report current recovery
> > > > status for udev") AER uses the result of error_detected() as parameter
> > > > to pci_uevent_ers(). As pci_uevent_ers() however does not handle
> > > > PCI_ERS_RESULT_NEED_RESET this results in a missing uevent for the
> > > > beginning of recovery if drivers request a reset. Fix this by treating
> > > > PCI_ERS_RESULT_NEED_RESET as beginning recovery.
> > > [...]
> > > > +++ b/drivers/pci/pci-driver.c
> > > > @@ -1592,6 +1592,7 @@ void pci_uevent_ers(struct pci_dev *pdev, enum pci_ers_result err_type)
> > > > switch (err_type) {
> > > > case PCI_ERS_RESULT_NONE:
> > > > case PCI_ERS_RESULT_CAN_RECOVER:
> > > > + case PCI_ERS_RESULT_NEED_RESET:
> > > > envp[idx++] = "ERROR_EVENT=BEGIN_RECOVERY";
> > > > envp[idx++] = "DEVICE_ONLINE=0";
> > > > break;
> > >
> > > I note that PCI_ERS_RESULT_NO_AER_DRIVER is also missing in that
> > > switch/case statement. I guess for the patch to be complete,
> > > it needs to be added to the PCI_ERS_RESULT_DISCONNECT case.
> > > Do you agree?
As far as I can see PCI_ERS_RESULT_NO_AER_DRIVER only occurs in the AER
code and leads to abandoning all recovery attempts for the whole
subtree with a disconnect. So my thinking is that the uevent is just
disconnect. That also matches your proposal below. Thankfully it looks
like there are still kernel messages indicating the reason of the
failure.
> >
> > I realize now there's a bigger problem here: In pcie_do_recovery(),
> > when control reaches the "failed:" label, a uevent is only signaled
> > for the *bridge*. Shouldn't a uevent instead be signaled for every
> > device *below* the bridge? (And possibly the bridge itself if it was
> > the device reporting the error.)
>
> The small patch below should resolve this issue.
> Please let me know what you think.
The patch makes sense to me I agree one should get uevents for each
downstream device. Please Cc me when you send it.
>
> > In that case you don't need to add PCI_ERS_RESULT_NO_AER_DRIVER to
> > the switch/case statement because we wouldn't want to have multiple
> > uevents reporting disconnect, so the one emitted below the "failed:"
> > label would be sufficient.
>
> I'll send a separate Reviewed-by for your original patch as the small
> patch below should resolve my concern about PCI_ERS_RESULT_NO_AER_DRIVER.
>
> > This all looks so broken that I'm starting to wonder if there's any
> > user space application at all that takes advantage of these uevents?
>
> I'd still be interested to know which user space application you're
> using to track these uevents?
>
> Thanks,
>
> Lukas
Thanks for the R-b! And yes I agree we should minimize the differences
between the behavior of the implementations. I'll see if I can sync up
on this with Mahesh too.
I only tested this with udevadm so far. That said I had multiple
projects ask for ways to monitor for errors/recovery from user-space so
at least for s390 there is interest for these events. There is also a
bit of mainframe specifics here of course, for example our machines
have the ability to fail over a broken PCI link to an I/O drawer with
an alternate path and that piggybacks on these recovery mechanisms. And
generally, we're looking at expanding our support for and use of PCI
error recovery and will hopefully be sharing some more of that soon.
Also note that with the recently released IBM z17 and IBM LinuxONE 5
with Linux we're significantly expanding our reliance on native
PCI[0][1].
[0]
https://www.ibm.com/docs/en/linux-on-systems?topic=networking-network-express
[1]
https://vmworkshop.org/2025/present/9175tovr.pdf
>
On 7/31/25 6:01 AM, Lukas Wunner wrote:
> On Wed, Jul 30, 2025 at 10:24:07PM +0200, Lukas Wunner wrote:
>> On Wed, Jul 30, 2025 at 10:01:50PM +0200, Lukas Wunner wrote:
>>> On Wed, Jul 30, 2025 at 01:20:57PM +0200, Niklas Schnelle wrote:
>>>> Since commit 7b42d97e99d3 ("PCI/ERR: Always report current recovery
>>>> status for udev") AER uses the result of error_detected() as parameter
>>>> to pci_uevent_ers(). As pci_uevent_ers() however does not handle
>>>> PCI_ERS_RESULT_NEED_RESET this results in a missing uevent for the
>>>> beginning of recovery if drivers request a reset. Fix this by treating
>>>> PCI_ERS_RESULT_NEED_RESET as beginning recovery.
>>> [...]
>>>> +++ b/drivers/pci/pci-driver.c
>>>> @@ -1592,6 +1592,7 @@ void pci_uevent_ers(struct pci_dev *pdev, enum pci_ers_result err_type)
>>>> switch (err_type) {
>>>> case PCI_ERS_RESULT_NONE:
>>>> case PCI_ERS_RESULT_CAN_RECOVER:
>>>> + case PCI_ERS_RESULT_NEED_RESET:
>>>> envp[idx++] = "ERROR_EVENT=BEGIN_RECOVERY";
>>>> envp[idx++] = "DEVICE_ONLINE=0";
>>>> break;
>>> I note that PCI_ERS_RESULT_NO_AER_DRIVER is also missing in that
>>> switch/case statement. I guess for the patch to be complete,
>>> it needs to be added to the PCI_ERS_RESULT_DISCONNECT case.
>>> Do you agree?
>> I realize now there's a bigger problem here: In pcie_do_recovery(),
>> when control reaches the "failed:" label, a uevent is only signaled
>> for the *bridge*. Shouldn't a uevent instead be signaled for every
>> device *below* the bridge? (And possibly the bridge itself if it was
>> the device reporting the error.)
> The small patch below should resolve this issue.
> Please let me know what you think.
>
>> In that case you don't need to add PCI_ERS_RESULT_NO_AER_DRIVER to
>> the switch/case statement because we wouldn't want to have multiple
>> uevents reporting disconnect, so the one emitted below the "failed:"
>> label would be sufficient.
> I'll send a separate Reviewed-by for your original patch as the small
> patch below should resolve my concern about PCI_ERS_RESULT_NO_AER_DRIVER.
>
>> This all looks so broken that I'm starting to wonder if there's any
>> user space application at all that takes advantage of these uevents?
> I'd still be interested to know which user space application you're
> using to track these uevents?
>
> Thanks,
>
> Lukas
>
> -- >8 --
>
> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
> index e795e5ae..3a95aa2 100644
> --- a/drivers/pci/pcie/err.c
> +++ b/drivers/pci/pcie/err.c
> @@ -165,6 +165,12 @@ static int report_resume(struct pci_dev *dev, void *data)
> return 0;
> }
>
> +static int report_disconnect(struct pci_dev *dev, void *data)
> +{
> + pci_uevent_ers(dev, PCI_ERS_RESULT_DISCONNECT);
> + return 0;
> +}
Since you are notifying the user space, I am wondering whether the drivers
should be notified about the recovery failure?
> +
> /**
> * pci_walk_bridge - walk bridges potentially AER affected
> * @bridge: bridge which may be a Port, an RCEC, or an RCiEP
> @@ -272,7 +278,7 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
> failed:
> pci_walk_bridge(bridge, pci_pm_runtime_put, NULL);
>
> - pci_uevent_ers(bridge, PCI_ERS_RESULT_DISCONNECT);
> + pci_walk_bridge(bridge, report_disconnect, NULL);
>
> pci_info(bridge, "device recovery failed\n");
>
>
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
On Thu, Jul 31, 2025 at 10:04:38AM -0700, Sathyanarayanan Kuppuswamy wrote:
> On 7/31/25 6:01 AM, Lukas Wunner wrote:
> > +++ b/drivers/pci/pcie/err.c
> > @@ -165,6 +165,12 @@ static int report_resume(struct pci_dev *dev, void *data)
> > return 0;
> > }
> > +static int report_disconnect(struct pci_dev *dev, void *data)
> > +{
> > + pci_uevent_ers(dev, PCI_ERS_RESULT_DISCONNECT);
> > + return 0;
> > +}
>
> Since you are notifying the user space, I am wondering whether the drivers
> should be notified about the recovery failure?
The drivers are usually *causing* the recovery failure by returning
PCI_ERS_RESULT_DISCONNECT from their pci_error_handlers callbacks
(or by lacking pci_error_handlers, in particular ->error_detected()).
So in principle the drivers should be aware of recovery failure.
There are cases where multiple drivers are involved. E.g. on GPUs,
there's often a PCIe switch with a graphics device and various sound
or telemetry devices. Typically errors are reported by the Upstream
Port, so the Secondary Bus Reset occurs at the Root or Downstream Port
above the Upstream Port and affects the switch and all subordinate
devices. In cases like this, recovery failure may be caused by a
single driver (e.g. GPU) and the other drivers (e.g. telemetry) may
be unaware of it.
The recovery flow documented in Documentation/PCI/pci-error-recovery.rst
was originally conceived for EEH and indeed EEH does notify all drivers
of recovery failures by invoking the ->error_detected() callback with
channel_state pci_channel_io_perm_failure. See this call ...
eeh_pe_report("error_detected(permanent failure)", pe,
eeh_report_failure, NULL);
... in arch/powerpc/kernel/eeh_driver.c below the recover_failed label
in eeh_handle_normal_event().
I don't know why pcie_do_recovery() doesn't do the same on recovery
failure. This is one of several annoying deviations between AER and
EEH. Ideally the behavior should be the same across all platforms
so that drivers don't have to cope with platform-specific quirks.
However I think that's orthogonal to the pci_uevent_ers() invocation
in pcie_do_recovery().
Thanks,
Lukas
Hi Lukas,
On 7/31/25 10:44 PM, Lukas Wunner wrote:
> On Thu, Jul 31, 2025 at 10:04:38AM -0700, Sathyanarayanan Kuppuswamy wrote:
>> On 7/31/25 6:01 AM, Lukas Wunner wrote:
>>> +++ b/drivers/pci/pcie/err.c
>>> @@ -165,6 +165,12 @@ static int report_resume(struct pci_dev *dev, void *data)
>>> return 0;
>>> }
>>> +static int report_disconnect(struct pci_dev *dev, void *data)
>>> +{
>>> + pci_uevent_ers(dev, PCI_ERS_RESULT_DISCONNECT);
>>> + return 0;
>>> +}
>> Since you are notifying the user space, I am wondering whether the drivers
>> should be notified about the recovery failure?
> The drivers are usually *causing* the recovery failure by returning
> PCI_ERS_RESULT_DISCONNECT from their pci_error_handlers callbacks
> (or by lacking pci_error_handlers, in particular ->error_detected()).
>
> So in principle the drivers should be aware of recovery failure.
>
> There are cases where multiple drivers are involved. E.g. on GPUs,
> there's often a PCIe switch with a graphics device and various sound
> or telemetry devices. Typically errors are reported by the Upstream
> Port, so the Secondary Bus Reset occurs at the Root or Downstream Port
> above the Upstream Port and affects the switch and all subordinate
> devices. In cases like this, recovery failure may be caused by a
> single driver (e.g. GPU) and the other drivers (e.g. telemetry) may
> be unaware of it.
Yes, my comment was referring to the scenario mentioned above. If one of the
subordinate devices fails recovery, then recovery effectively fails for all devices
under that downstream port (or root port). Notifying all devices under that port
would allow their drivers to perform the necessary cleanup
>
> The recovery flow documented in Documentation/PCI/pci-error-recovery.rst
> was originally conceived for EEH and indeed EEH does notify all drivers
> of recovery failures by invoking the ->error_detected() callback with
> channel_state pci_channel_io_perm_failure. See this call ...
>
> eeh_pe_report("error_detected(permanent failure)", pe,
> eeh_report_failure, NULL);
>
> ... in arch/powerpc/kernel/eeh_driver.c below the recover_failed label
> in eeh_handle_normal_event().
Agree. The current implementation does not seem to follow the steps
mentioned in the Documentation/PCI/pci-error-recovery.rst.
STEP 6: Permanent Failure
-------------------------
A "permanent failure" has occurred, and the platform cannot recover
the device. The platform will call error_detected() with a
pci_channel_state_t value of pci_channel_io_perm_failure.
The device driver should, at this point, assume the worst. It should
cancel all pending I/O, refuse all new I/O, returning -EIO to
higher layers. The device driver should then clean up all of its
memory and remove itself from kernel operations, much as it would
during system shutdown.
>
> I don't know why pcie_do_recovery() doesn't do the same on recovery
> failure. This is one of several annoying deviations between AER and
> EEH. Ideally the behavior should be the same across all platforms
> so that drivers don't have to cope with platform-specific quirks.
>
> However I think that's orthogonal to the pci_uevent_ers() invocation
> in pcie_do_recovery().
Agree. My thought is, since there is an attempt to fix the user notification
side of things, may be the driver side should also be fixed together .
> Thanks,
>
> Lukas
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
© 2016 - 2025 Red Hat, Inc.