PCI/ERR: s390/pci: Use pci_uevent_ers() in PCI recovery

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.

Quoted 6 lines · 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.

[...]

drivers/pci/pci-driver.c void pci_uevent_ers(struct pci_dev *pdev, enum pci_ers_result err_type) · line 1598

Quoted 9 lines

+++ b/drivers/pci/pci-driver.c

drivers/pci/pci-driver.c +1 -0

@@ -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?

Quoted 8 lines · Lukas Wunner

Quoted 6 lines · 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.

[...]

Quoted 14 lines

Review void pci_uevent_ers(struct pci_dev *pdev, enum pci_ers_result err_type) · line 1598

Quoted 9 lines

+++ b/drivers/pci/pci-driver.c

(quoted diff) +1 -0

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

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?

Quoted 22 lines · Lukas Wunner

Quoted 8 lines · Lukas Wunner

Quoted 6 lines · 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.

[...]

Quoted 13 lines

Review void pci_uevent_ers(struct pci_dev *pdev, enum pci_ers_result err_type) · line 1598

Quoted 9 lines

+++ b/drivers/pci/pci-driver.c

(quoted diff) +1 -0

@@ -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?

Quoted 5 lines

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.

Quoted 4 lines

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.

Quoted 2 lines

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

drivers/pci/pcie/err.c +0 -0

index e795e5ae..3a95aa2 100644

drivers/pci/pcie/err.c +2 -1

+
  /**
   * 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");
 

drivers/pci/pcie/err.c static int report_resume(struct pci_dev *dev, void *data) · line 171

Quoted 9 lines · Lukas Wunner

+++ b/drivers/pci/pcie/err.c

drivers/pci/pcie/err.c +5 -0

@@ -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?

Quoted 12 lines · Sathyanarayanan Kuppuswamy

drivers/pci/pcie/err.c static int report_resume(struct pci_dev *dev, void *data) · line 171

Quoted 9 lines · Lukas Wunner

+++ b/drivers/pci/pcie/err.c

drivers/pci/pcie/err.c +5 -0

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

Quoted 9 lines · Lukas Wunner

Quoted 8 lines · Lukas Wunner

Quoted 6 lines · 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.

[...]

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

+++ b/drivers/pci/pci-driver.c

(quoted diff) +1 -0

@@ -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:

Quoted 8 lines

Quoted 8 lines

Review void pci_uevent_ers(struct pci_dev *pdev, enum pci_ers_result err_type) · line 1598

Quoted 3 lines

(quoted diff) +0 -0

 		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?

Quoted 6 lines

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.

Quoted 5 lines

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.

Quoted 2 lines

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

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 [email protected]

Fixes 7b42d97e99d3 ("PCI/ERR: Always report current recovery status for udev")

Signed-off-by Niklas Schnelle <[email protected]>

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

Signed-off-by Niklas Schnelle <[email protected]>