[PATCH v3 0/2] PCI/ERR: s390/pci: Use pci_uevent_ers() in PCI recovery

[PATCH v3 0/2] PCI/ERR: s390/pci: Use pci_uevent_ers() in PCI recovery

[Niklas Schnelle]

Hi Bjorn, Lukas, Mahesh,

This series adds issuing of uevents during PCI recovery on s390. In
developing this I noticed that pci_uevent_ers() ignores
PCI_ERS_RESULT_NEED_RESET. I think this will result in AER not generating a uevent
at the beginning of recovery if drivers request a reset via the voting
on error_detected() returns. This is fixed in the first patch and relied
upon by the s390 recovery code as it also uses the result of
error_detected() though with one device/driver at a time.

Thanks,
Niklas

Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
---
Changes in v3:
- Reworded cover letter
- Rebase on v6.16
- Link to v2: https://lore.kernel.org/r/20250623-add_err_uevents-v2-0-a3a2cf8e711d@linux.ibm.com

Changes in v2:
- Add a patch fixing pci_uevent_ers() mistakenly ignoring PCI_ERS_RESULT_NEED_RESET
- Use the result of error_detected() for initial pci_uevent_ers()
- Drop fixes tag in s390 patch
- Rebase and re-test on current master
- Link to v1: https://lore.kernel.org/r/20250424-add_err_uevents-v1-1-3384d6b779c6@linux.ibm.com

---
Niklas Schnelle (2):
      PCI/AER: Fix missing uevent on recovery when a reset is requested
      PCI/ERR: s390/pci: Use pci_uevent_ers() in PCI recovery

 arch/s390/pci/pci_event.c | 3 +++
 drivers/pci/pci-driver.c  | 3 ++-
 include/linux/pci.h       | 2 +-
 3 files changed, 6 insertions(+), 2 deletions(-)
---
base-commit: 038d61fd642278bab63ee8ef722c50d10ab01e8f
change-id: 20250417-add_err_uevents-6f8d4d7ce09c

Best regards,
-- 
Niklas Schnelle

[PATCH v3 1/2] PCI/AER: Fix missing uevent on recovery when a reset is requested

[Niklas Schnelle]

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.1

[PATCH v3 2/2] PCI/ERR: s390/pci: Use pci_uevent_ers() in PCI recovery

[Niklas Schnelle]

Issue uevents during PCI recovery using pci_uevent_ers() as done by EEH
and AER PCIe recovery routines.

Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
---
 arch/s390/pci/pci_event.c | 3 +++
 drivers/pci/pci-driver.c  | 2 +-
 include/linux/pci.h       | 2 +-
 3 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/s390/pci/pci_event.c b/arch/s390/pci/pci_event.c
index d930416d4c903f709764b75fe45fb66e529fcc5b..b95376041501f479eee20705d45fb8c68553da71 100644
--- a/arch/s390/pci/pci_event.c
+++ b/arch/s390/pci/pci_event.c
@@ -88,6 +88,7 @@ static pci_ers_result_t zpci_event_notify_error_detected(struct pci_dev *pdev,
 	pci_ers_result_t ers_res = PCI_ERS_RESULT_DISCONNECT;
 
 	ers_res = driver->err_handler->error_detected(pdev,  pdev->error_state);
+	pci_uevent_ers(pdev, ers_res);
 	if (ers_result_indicates_abort(ers_res))
 		pr_info("%s: Automatic recovery failed after initial reporting\n", pci_name(pdev));
 	else if (ers_res == PCI_ERS_RESULT_NEED_RESET)
@@ -244,6 +245,7 @@ static pci_ers_result_t zpci_event_attempt_error_recovery(struct pci_dev *pdev)
 		ers_res = PCI_ERS_RESULT_RECOVERED;
 
 	if (ers_res != PCI_ERS_RESULT_RECOVERED) {
+		pci_uevent_ers(pdev, PCI_ERS_RESULT_DISCONNECT);
 		pr_err("%s: Automatic recovery failed; operator intervention is required\n",
 		       pci_name(pdev));
 		status_str = "failed (driver can't recover)";
@@ -253,6 +255,7 @@ static pci_ers_result_t zpci_event_attempt_error_recovery(struct pci_dev *pdev)
 	pr_info("%s: The device is ready to resume operations\n", pci_name(pdev));
 	if (driver->err_handler->resume)
 		driver->err_handler->resume(pdev);
+	pci_uevent_ers(pdev, PCI_ERS_RESULT_RECOVERED);
 out_unlock:
 	pci_dev_unlock(pdev);
 	zpci_report_status(zdev, "recovery", status_str);
diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 94ba6938b7c6271b557cc7f17ffb89631d83827e..2f3037050fd011108ef93e39d2d78a5c7e22fd05 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -1578,7 +1578,7 @@ static int pci_uevent(const struct device *dev, struct kobj_uevent_env *env)
 	return 0;
 }
 
-#if defined(CONFIG_PCIEAER) || defined(CONFIG_EEH)
+#if defined(CONFIG_PCIEAER) || defined(CONFIG_EEH) || defined(CONFIG_S390)
 /**
  * pci_uevent_ers - emit a uevent during recovery path of PCI device
  * @pdev: PCI device undergoing error recovery
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 05e68f35f39238f8b9ce08df97b384d1c1e89bbe..bcc412a21d93a6dcc566f011258ed39d80d896c2 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -2737,7 +2737,7 @@ static inline bool pci_is_thunderbolt_attached(struct pci_dev *pdev)
 	return false;
 }
 
-#if defined(CONFIG_PCIEPORTBUS) || defined(CONFIG_EEH)
+#if defined(CONFIG_PCIEPORTBUS) || defined(CONFIG_EEH) || defined(CONFIG_S390)
 void pci_uevent_ers(struct pci_dev *pdev, enum  pci_ers_result err_type);
 #endif
 

-- 
2.48.1

Re: [PATCH v3 1/2] PCI/AER: Fix missing uevent on recovery when a reset is requested

[Lukas Wunner]

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

Re: [PATCH v3 1/2] PCI/AER: Fix missing uevent on recovery when a reset is requested

[Lukas Wunner]

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

Re: [PATCH v3 2/2] PCI/ERR: s390/pci: Use pci_uevent_ers() in PCI recovery

[Lukas Wunner]

On Wed, Jul 30, 2025 at 01:20:58PM +0200, Niklas Schnelle wrote:
> Issue uevents during PCI recovery using pci_uevent_ers() as done by EEH
> and AER PCIe recovery routines.
> 
> Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>

Reviewed-by: Lukas Wunner <lukas@wunner.de>

Re: [PATCH v3 1/2] PCI/AER: Fix missing uevent on recovery when a reset is requested

[Lukas Wunner]

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");
 

Re: [PATCH v3 1/2] PCI/AER: Fix missing uevent on recovery when a reset is requested

[Lukas Wunner]

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.
> 
> 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>

Re: [PATCH v3 1/2] PCI/AER: Fix missing uevent on recovery when a reset is requested

[Sathyanarayanan Kuppuswamy]

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

Re: [PATCH v3 1/2] PCI/AER: Fix missing uevent on recovery when a reset is requested

[Lukas Wunner]

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

Re: [PATCH v3 1/2] PCI/AER: Fix missing uevent on recovery when a reset is requested

[Sathyanarayanan Kuppuswamy]

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

Re: [PATCH v3 1/2] PCI/AER: Fix missing uevent on recovery when a reset is requested

[Niklas Schnelle]

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
>