Add Error Disconnect Recover (EDR) support

From: Kuppuswamy Sathyanarayanan <[email protected]>

Commit bdb5ac85777d ("PCI/ERR: Handle fatal error recovery") uses reset_link() to recover from fatal errors. But, if the reset is successful there is no need to continue the rest of the error recovery checks. Also, during fatal error recovery, if the initial value of error status is PCI_ERS_RESULT_DISCONNECT or PCI_ERS_RESULT_NO_AER_DRIVER then even after successful recovery (using reset_link()) pcie_do_recovery() will report the recovery result as failure. So update the status of error after reset_link().

Fixes bdb5ac85777d ("PCI/ERR: Handle fatal error recovery")

Cc Ashok Raj <[email protected]>

Cc Keith Busch <[email protected]>

Signed-off-by Kuppuswamy Sathyanarayanan <[email protected]>

Acked-by Keith Busch <[email protected]>

---
 drivers/pci/pcie/err.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

drivers/pci/pcie/err.c +7 -3

index b0e6048a9208..53cd9200ec2c 100644
@@ -204,9 +204,12 @@ void pcie_do_recovery(struct pci_dev *dev, enum pci_channel_state state,
 	else
 		pci_walk_bus(bus, report_normal_detected, &status);
 
-	if (state == pci_channel_io_frozen &&
-	    reset_link(dev, service) != PCI_ERS_RESULT_RECOVERED)
-		goto failed;
+	if (state == pci_channel_io_frozen) {
+		status = reset_link(dev, service);
+		if (status != PCI_ERS_RESULT_RECOVERED)
+			goto failed;
+		goto done;
+	}
 
 	if (status == PCI_ERS_RESULT_CAN_RECOVER) {
 		status = PCI_ERS_RESULT_RECOVERED;

@@ -228,6 +231,7 @@ void pcie_do_recovery(struct pci_dev *dev, enum pci_channel_state state,
 	if (status != PCI_ERS_RESULT_RECOVERED)
 		goto failed;
 
+done:
 	pci_dbg(dev, "broadcast resume message\n");
 	pci_walk_bus(bus, report_resume, &status);
 

Signature · 1 line

2.21.0

Quoted 10 lines · On Thu, Dec 26, 2019 at 04:39:07PM -0800…

From: Kuppuswamy Sathyanarayanan <[email protected]>

Commit bdb5ac85777d ("PCI/ERR: Handle fatal error recovery") uses reset_link() to recover from fatal errors. But, if the reset is successful there is no need to continue the rest of the error recovery checks. Also, during fatal error recovery, if the initial value of error status is PCI_ERS_RESULT_DISCONNECT or PCI_ERS_RESULT_NO_AER_DRIVER then even after successful recovery (using reset_link()) pcie_do_recovery() will report the recovery result as failure. So update the status of error after reset_link().

I like the part about updating "status" with the result of reset_link(), and I split that into its own patch because it seems like a fix that *can* be separated.

But I'm not convinced that we should skip the ->slot_reset() callbacks if the reset_link() was successful.

According to
Documentation/PCI/pci-error-recovery.rst, we should call ->slot_reset() after completion of the reset.

For example, rsxx_err_handler implements ->slot_reset(), but not ->resume(). If we reset the device, we'll claim success and return, but we won't call rsxx_slot_reset(), which does a bunch of important-looking recovery stuff.

If pci-error-recovery.rst is wrong, we should fix that (after auditing all the drivers to make sure they match).

Quoted 40 lines

Fixes bdb5ac85777d ("PCI/ERR: Handle fatal error recovery")

Cc Ashok Raj <[email protected]>

Cc Keith Busch <[email protected]>

Signed-off-by Kuppuswamy Sathyanarayanan <[email protected]>

Acked-by Keith Busch <[email protected]>

---
  drivers/pci/pcie/err.c | 10 +++++++---
  1 file changed, 7 insertions(+), 3 deletions(-)

drivers/pci/pcie/err.c +7 -3

index b0e6048a9208..53cd9200ec2c 100644
@@ -204,9 +204,12 @@ void pcie_do_recovery(struct pci_dev *dev, enum pci_channel_state state,
  	else
  		pci_walk_bus(bus, report_normal_detected, &status);
 
-	if (state == pci_channel_io_frozen &&
-	    reset_link(dev, service) != PCI_ERS_RESULT_RECOVERED)
-		goto failed;
+	if (state == pci_channel_io_frozen) {
+		status = reset_link(dev, service);
+		if (status != PCI_ERS_RESULT_RECOVERED)
+			goto failed;
+		goto done;
+	}
 
  	if (status == PCI_ERS_RESULT_CAN_RECOVER) {
  		status = PCI_ERS_RESULT_RECOVERED;

@@ -228,6 +231,7 @@ void pcie_do_recovery(struct pci_dev *dev, enum pci_channel_state state,
  	if (status != PCI_ERS_RESULT_RECOVERED)
  		goto failed;
 
+done:
  	pci_dbg(dev, "broadcast resume message\n");
  	pci_walk_bus(bus, report_resume, &status);
 

Signature · 1 line

2.21.0

Quoted 15 lines · Bjorn Helgaas

Quoted 10 lines · On Thu, Dec 26, 2019 at 04:39:07PM -0800…

From: Kuppuswamy Sathyanarayanan <[email protected]>

Commit bdb5ac85777d ("PCI/ERR: Handle fatal error recovery") uses reset_link() to recover from fatal errors. But, if the reset is successful there is no need to continue the rest of the error recovery checks. Also, during fatal error recovery, if the initial value of error status is PCI_ERS_RESULT_DISCONNECT or PCI_ERS_RESULT_NO_AER_DRIVER then even after successful recovery (using reset_link()) pcie_do_recovery() will report the recovery result as failure. So update the status of error after reset_link().

I like the part about updating "status" with the result of reset_link(), and I split that into its own patch because it seems like a fix that *can* be separated.

Quoted 2 lines

But I'm not convinced that we should skip the ->slot_reset() callbacks if the reset_link() was successful.

If reset_link() call is successful then the result value will be "PCI_ERS_RESULT_RECOVERED". So even if you proceed with rest of the code, slot_reset() will never get called right ?

Quoted 48 lines

According to Documentation/PCI/pci-error-recovery.rst, we should call ->slot_reset() after completion of the reset.

For example, rsxx_err_handler implements ->slot_reset(), but not ->resume(). If we reset the device, we'll claim success and return, but we won't call rsxx_slot_reset(), which does a bunch of important-looking recovery stuff.

If pci-error-recovery.rst is wrong, we should fix that (after auditing all the drivers to make sure they match).

Quoted 36 lines

Fixes bdb5ac85777d ("PCI/ERR: Handle fatal error recovery")

Cc Ashok Raj <[email protected]>

Cc Keith Busch <[email protected]>

Signed-off-by Kuppuswamy Sathyanarayanan <[email protected]>

Acked-by Keith Busch <[email protected]>

---
  drivers/pci/pcie/err.c | 10 +++++++---
  1 file changed, 7 insertions(+), 3 deletions(-)

drivers/pci/pcie/err.c +7 -3

index b0e6048a9208..53cd9200ec2c 100644
@@ -204,9 +204,12 @@ void pcie_do_recovery(struct pci_dev *dev, enum pci_channel_state state,
  	else
  		pci_walk_bus(bus, report_normal_detected, &status);
-	if (state == pci_channel_io_frozen &&
-	    reset_link(dev, service) != PCI_ERS_RESULT_RECOVERED)
-		goto failed;
+	if (state == pci_channel_io_frozen) {
+		status = reset_link(dev, service);
+		if (status != PCI_ERS_RESULT_RECOVERED)
+			goto failed;
+		goto done;
+	}
  	if (status == PCI_ERS_RESULT_CAN_RECOVER) {
  		status = PCI_ERS_RESULT_RECOVERED;

@@ -228,6 +231,7 @@ void pcie_do_recovery(struct pci_dev *dev, enum pci_channel_state state,
  	if (status != PCI_ERS_RESULT_RECOVERED)
  		goto failed;
+done:
  	pci_dbg(dev, "broadcast resume message\n");
  	pci_walk_bus(bus, report_resume, &status);

Signature · 1 line

2.21.0

Signature · 2 lines

Sathyanarayanan Kuppuswamy
Linux kernel developer

Quoted 16 lines · Kuppuswamy Sathyanarayanan

Quoted 15 lines · Bjorn Helgaas

Quoted 10 lines · On Thu, Dec 26, 2019 at 04:39:07PM -0800…

From: Kuppuswamy Sathyanarayanan <[email protected]>

Commit bdb5ac85777d ("PCI/ERR: Handle fatal error recovery") uses reset_link() to recover from fatal errors. But, if the reset is successful there is no need to continue the rest of the error recovery checks. Also, during fatal error recovery, if the initial value of error status is PCI_ERS_RESULT_DISCONNECT or PCI_ERS_RESULT_NO_AER_DRIVER then even after successful recovery (using reset_link()) pcie_do_recovery() will report the recovery result as failure. So update the status of error after reset_link().

I like the part about updating "status" with the result of reset_link(), and I split that into its own patch because it seems like a fix that *can* be separated.

Quoted 5 lines

Quoted 2 lines

But I'm not convinced that we should skip the ->slot_reset() callbacks if the reset_link() was successful.

If reset_link() call is successful then the result value will be "PCI_ERS_RESULT_RECOVERED". So even if you proceed with rest of the code, slot_reset() will never get called right ?

The current code:

         if (state == pci_channel_io_frozen &&
             reset_link(dev, service) != PCI_ERS_RESULT_RECOVERED)
                 goto failed;
         ...
         if (status == PCI_ERS_RESULT_NEED_RESET) {
                 status = PCI_ERS_RESULT_RECOVERED;
                 pci_walk_bus(bus, report_slot_reset, &status);

doesn't save the result of reset_link(), so if status was PCI_ERS_RESULT_NEED_RESET and the reset succeeds, we will call ->slot_reset().

After your patch, if "state == pci_channel_io_frozen", we *never* call ->slot_reset().

Do you think that matches pci-error-recovery.rst? It doesn't seem like it to me, but perhaps I haven't read it closely enough.

Quoted 53 lines

Quoted 49 lines

According to Documentation/PCI/pci-error-recovery.rst, we should call ->slot_reset() after completion of the reset.

For example, rsxx_err_handler implements ->slot_reset(), but not ->resume(). If we reset the device, we'll claim success and return, but we won't call rsxx_slot_reset(), which does a bunch of important-looking recovery stuff.

If pci-error-recovery.rst is wrong, we should fix that (after auditing all the drivers to make sure they match).

Quoted 36 lines

Fixes bdb5ac85777d ("PCI/ERR: Handle fatal error recovery")

Cc Ashok Raj <[email protected]>

Cc Keith Busch <[email protected]>

Signed-off-by Kuppuswamy Sathyanarayanan <[email protected]>

Acked-by Keith Busch <[email protected]>

---
   drivers/pci/pcie/err.c | 10 +++++++---
   1 file changed, 7 insertions(+), 3 deletions(-)

drivers/pci/pcie/err.c +7 -3

index b0e6048a9208..53cd9200ec2c 100644
@@ -204,9 +204,12 @@ void pcie_do_recovery(struct pci_dev *dev, enum pci_channel_state state,
   	else
   		pci_walk_bus(bus, report_normal_detected, &status);
-	if (state == pci_channel_io_frozen &&
-	    reset_link(dev, service) != PCI_ERS_RESULT_RECOVERED)
-		goto failed;
+	if (state == pci_channel_io_frozen) {
+		status = reset_link(dev, service);
+		if (status != PCI_ERS_RESULT_RECOVERED)
+			goto failed;
+		goto done;
+	}
   	if (status == PCI_ERS_RESULT_CAN_RECOVER) {
   		status = PCI_ERS_RESULT_RECOVERED;

@@ -228,6 +231,7 @@ void pcie_do_recovery(struct pci_dev *dev, enum pci_channel_state state,
   	if (status != PCI_ERS_RESULT_RECOVERED)
   		goto failed;
+done:
   	pci_dbg(dev, "broadcast resume message\n");
   	pci_walk_bus(bus, report_resume, &status);

Signature · 1 line

2.21.0

Signature · 2 lines

Sathyanarayanan Kuppuswamy
Linux kernel developer

Quoted 39 lines · Bjorn Helgaas

Quoted 16 lines · Kuppuswamy Sathyanarayanan

Quoted 15 lines · Bjorn Helgaas

Quoted 10 lines · On Thu, Dec 26, 2019 at 04:39:07PM -0800…

From: Kuppuswamy Sathyanarayanan <[email protected]>

Commit bdb5ac85777d ("PCI/ERR: Handle fatal error recovery") uses reset_link() to recover from fatal errors. But, if the reset is successful there is no need to continue the rest of the error recovery checks. Also, during fatal error recovery, if the initial value of error status is PCI_ERS_RESULT_DISCONNECT or PCI_ERS_RESULT_NO_AER_DRIVER then even after successful recovery (using reset_link()) pcie_do_recovery() will report the recovery result as failure. So update the status of error after reset_link().

I like the part about updating "status" with the result of reset_link(), and I split that into its own patch because it seems like a fix that *can* be separated.

Quoted 5 lines

Quoted 2 lines

But I'm not convinced that we should skip the ->slot_reset() callbacks if the reset_link() was successful.

If reset_link() call is successful then the result value will be "PCI_ERS_RESULT_RECOVERED". So even if you proceed with rest of the code, slot_reset() will never get called right ?

The current code:

         if (state == pci_channel_io_frozen &&
             reset_link(dev, service) != PCI_ERS_RESULT_RECOVERED)
                 goto failed;
         ...
         if (status == PCI_ERS_RESULT_NEED_RESET) {
                 status = PCI_ERS_RESULT_RECOVERED;
                 pci_walk_bus(bus, report_slot_reset, &status);

doesn't save the result of reset_link(), so if status was PCI_ERS_RESULT_NEED_RESET and the reset succeeds, we will call ->slot_reset().

After your patch, if "state == pci_channel_io_frozen", we *never* call ->slot_reset().

Quoted 2 lines

Do you think that matches pci-error-recovery.rst? It doesn't seem like it to me, but perhaps I haven't read it closely enough.

Documentation does not have clear details on what to do with return value of reset_link() (step 3). But IMO, if step 3 recovers the device and returns PCI_ERS_RESULT_RECOVERED then there is no need to proceed to slot reset (step 4). May be we should update the Documentation?

Quoted 52 lines

Quoted 52 lines

Quoted 48 lines

According to Documentation/PCI/pci-error-recovery.rst, we should call ->slot_reset() after completion of the reset.

For example, rsxx_err_handler implements ->slot_reset(), but not ->resume(). If we reset the device, we'll claim success and return, but we won't call rsxx_slot_reset(), which does a bunch of important-looking recovery stuff.

If pci-error-recovery.rst is wrong, we should fix that (after auditing all the drivers to make sure they match).

Quoted 36 lines

Fixes bdb5ac85777d ("PCI/ERR: Handle fatal error recovery")

Cc Ashok Raj <[email protected]>

Cc Keith Busch <[email protected]>

Signed-off-by Kuppuswamy Sathyanarayanan <[email protected]>

Acked-by Keith Busch <[email protected]>

---
   drivers/pci/pcie/err.c | 10 +++++++---
   1 file changed, 7 insertions(+), 3 deletions(-)

drivers/pci/pcie/err.c +7 -3

index b0e6048a9208..53cd9200ec2c 100644
@@ -204,9 +204,12 @@ void pcie_do_recovery(struct pci_dev *dev, enum pci_channel_state state,
   	else
   		pci_walk_bus(bus, report_normal_detected, &status);
-	if (state == pci_channel_io_frozen &&
-	    reset_link(dev, service) != PCI_ERS_RESULT_RECOVERED)
-		goto failed;
+	if (state == pci_channel_io_frozen) {
+		status = reset_link(dev, service);
+		if (status != PCI_ERS_RESULT_RECOVERED)
+			goto failed;
+		goto done;
+	}
   	if (status == PCI_ERS_RESULT_CAN_RECOVER) {
   		status = PCI_ERS_RESULT_RECOVERED;

@@ -228,6 +231,7 @@ void pcie_do_recovery(struct pci_dev *dev, enum pci_channel_state state,
   	if (status != PCI_ERS_RESULT_RECOVERED)
   		goto failed;
+done:
   	pci_dbg(dev, "broadcast resume message\n");
   	pci_walk_bus(bus, report_resume, &status);

Signature · 1 line

2.21.0

Signature · 2 lines

Sathyanarayanan Kuppuswamy
Linux kernel developer

Signature · 2 lines

Sathyanarayanan Kuppuswamy
Linux kernel developer

Quoted 40 lines · Kuppuswamy Sathyanarayanan

Quoted 39 lines · Bjorn Helgaas

Quoted 16 lines · Kuppuswamy Sathyanarayanan

Quoted 15 lines · Bjorn Helgaas

Quoted 10 lines · On Thu, Dec 26, 2019 at 04:39:07PM -0800…

From: Kuppuswamy Sathyanarayanan <[email protected]>

Commit bdb5ac85777d ("PCI/ERR: Handle fatal error recovery") uses reset_link() to recover from fatal errors. But, if the reset is successful there is no need to continue the rest of the error recovery checks. Also, during fatal error recovery, if the initial value of error status is PCI_ERS_RESULT_DISCONNECT or PCI_ERS_RESULT_NO_AER_DRIVER then even after successful recovery (using reset_link()) pcie_do_recovery() will report the recovery result as failure. So update the status of error after reset_link().

I like the part about updating "status" with the result of reset_link(), and I split that into its own patch because it seems like a fix that *can* be separated.

Quoted 5 lines

Quoted 2 lines

But I'm not convinced that we should skip the ->slot_reset() callbacks if the reset_link() was successful.

If reset_link() call is successful then the result value will be "PCI_ERS_RESULT_RECOVERED". So even if you proceed with rest of the code, slot_reset() will never get called right ?

The current code:

          if (state == pci_channel_io_frozen &&
              reset_link(dev, service) != PCI_ERS_RESULT_RECOVERED)
                  goto failed;
          ...
          if (status == PCI_ERS_RESULT_NEED_RESET) {
                  status = PCI_ERS_RESULT_RECOVERED;
                  pci_walk_bus(bus, report_slot_reset, &status);

doesn't save the result of reset_link(), so if status was PCI_ERS_RESULT_NEED_RESET and the reset succeeds, we will call ->slot_reset().

After your patch, if "state == pci_channel_io_frozen", we *never* call ->slot_reset().

Quoted 6 lines

Quoted 2 lines

Do you think that matches pci-error-recovery.rst? It doesn't seem like it to me, but perhaps I haven't read it closely enough.

Documentation does not have clear details on what to do with return value of reset_link() (step 3). But IMO, if step 3 recovers the device and returns PCI_ERS_RESULT_RECOVERED then there is no need to proceed to slot reset (step 4). May be we should update the Documentation?

Are you suggesting we don't need to call a driver callback after resetting the device? Note that the ->slot_reset() doesn't *perform* a reset; it is called *after* completion of a reset.

The doc says:

... Upon completion of slot reset, the platform will call the device slot_reset() callback.
...
This call gives drivers the chance to re-initialize the hardware (re-download firmware, etc.). At this point, the driver may assume that the card is in a fresh state and is fully functional. The slot is unfrozen and the driver has full access to PCI config space, memory mapped I/O space and DMA. Interrupts (Legacy, MSI, or MSI-X) will also be available.

After we reset a device, the driver certainly needs a chance to reinitialize it.

Quoted 56 lines

Quoted 52 lines

Quoted 52 lines

Quoted 48 lines

According to Documentation/PCI/pci-error-recovery.rst, we should call ->slot_reset() after completion of the reset.

For example, rsxx_err_handler implements ->slot_reset(), but not ->resume(). If we reset the device, we'll claim success and return, but we won't call rsxx_slot_reset(), which does a bunch of important-looking recovery stuff.

If pci-error-recovery.rst is wrong, we should fix that (after auditing all the drivers to make sure they match).

Quoted 36 lines

Fixes bdb5ac85777d ("PCI/ERR: Handle fatal error recovery")

Cc Ashok Raj <[email protected]>

Cc Keith Busch <[email protected]>

Signed-off-by Kuppuswamy Sathyanarayanan <[email protected]>

Acked-by Keith Busch <[email protected]>

---
    drivers/pci/pcie/err.c | 10 +++++++---
    1 file changed, 7 insertions(+), 3 deletions(-)

drivers/pci/pcie/err.c +7 -3

index b0e6048a9208..53cd9200ec2c 100644
@@ -204,9 +204,12 @@ void pcie_do_recovery(struct pci_dev *dev, enum pci_channel_state state,
    	else
    		pci_walk_bus(bus, report_normal_detected, &status);
-	if (state == pci_channel_io_frozen &&
-	    reset_link(dev, service) != PCI_ERS_RESULT_RECOVERED)
-		goto failed;
+	if (state == pci_channel_io_frozen) {
+		status = reset_link(dev, service);
+		if (status != PCI_ERS_RESULT_RECOVERED)
+			goto failed;
+		goto done;
+	}
    	if (status == PCI_ERS_RESULT_CAN_RECOVER) {
    		status = PCI_ERS_RESULT_RECOVERED;

@@ -228,6 +231,7 @@ void pcie_do_recovery(struct pci_dev *dev, enum pci_channel_state state,
    	if (status != PCI_ERS_RESULT_RECOVERED)
    		goto failed;
+done:
    	pci_dbg(dev, "broadcast resume message\n");
    	pci_walk_bus(bus, report_resume, &status);

Signature · 1 line

2.21.0

Signature · 2 lines

Sathyanarayanan Kuppuswamy
Linux kernel developer

Signature · 2 lines

Sathyanarayanan Kuppuswamy
Linux kernel developer

From: Kuppuswamy Sathyanarayanan <[email protected]>

As per PCI firmware specification r3.2 System Firmware Intermediary (SFI) _OSC and DPC Updates ECR
(https://members.pcisig.com/wg/PCI-SIG/document/13563),

sec titled
"DPC Event Handling Implementation Note", page 10, Error Disconnect Recover (EDR) support allows OS to handle error recovery and clearing Error Registers even in FF mode. So create exception for FF mode checks in pci_cleanup_aer_uncorrect_error_status(), pci_aer_clear_fatal_status() and pci_cleanup_aer_error_status_regs() functions when its being called from DPC code path.

[+cc Austin]

Quoted 5 lines · On Thu, Dec 26, 2019 at 04:39:11PM -0800…

From: Kuppuswamy Sathyanarayanan <[email protected]>

As per PCI firmware specification r3.2 System Firmware Intermediary (SFI) _OSC and DPC Updates ECR
(https://members.pcisig.com/wg/PCI-SIG/document/13563),

What is the state of this ECR? I see it in the "PCI Express Review Zone Archive". I don't know what the usage is of the "Review Zone" vs the "Review Zone Archive / PCI Express Review Zone Archive". AFAICS, it is not listed in any of the "Documents for 60 Day Member Review".

And I think it needs some clarification (for one thing, it needs to say what the red/blue text means). I've mentioned some other items to Austin, but I haven't read it in detail because it seems like it's not quite baked yet.

E.g., there's language about "it may make sense for an embedded system OS to own SFI, but it's recommended that general-purpose OSes never request SFI ownership." That's useless: Linux is certainly a general purpose OS, but Linux is also often an embedded OS. So the ECR doesn't provide useful guidance about how an OS should decide whether to request SFI ownership.

Making code changes based on a published spec or ECN is fine, obviously. Changes based on an ECR that is well on track to being accepted, e.g., is in the 60-day review period, are probably OK. I don't yet have warm fuzzies about this ECR because I have no idea how far along it is.

We might be able to justify some of these changes based on other specs; it just sounds weird to me to say "based on this Engineering Change Request that might be accepted someday, we must do X". Anybody can dream up an ECR that says anything at all, so AFAICT, an ECR is not at all authoritative.

Quoted 7 lines

sec titled
"DPC Event Handling Implementation Note", page 10, Error Disconnect Recover (EDR) support allows OS to handle error recovery and clearing Error Registers even in FF mode. So create exception for FF mode checks in pci_cleanup_aer_uncorrect_error_status(), pci_aer_clear_fatal_status() and pci_cleanup_aer_error_status_regs() functions when its being called from DPC code path.