PCI/AER: don't call recovery process for correctable errors

PATCH In review Tyler Baicar 2 participants 7 messages Aug 28, 2017 – Nov 17, 2017 PCI

Share & tools

Tyler Baicar

author

2017-08-28 17:09 UTC #0 lore raw

PATCH 1/1 PCI/AER: don't call recovery process for correctable errors

Correctable errors do not need any software intervention, so avoid calling into the software recovery process for correctable errors.

Signed-off-by Tyler Baicar <[email protected]>

---
 drivers/pci/pcie/aer/aerdrv_core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/pci/pcie/aer/aerdrv_core.c +2 -1

index b1303b3..4765c11 100644
@@ -626,7 +626,8 @@ static void aer_recover_work_func(struct work_struct *work)
 			continue;
 		}
 		cper_print_aer(pdev, entry.severity, entry.regs);
-		do_recovery(pdev, entry.severity);
+		if (entry.severity != AER_CORRECTABLE)
+			do_recovery(pdev, entry.severity);
 		pci_dev_put(pdev);
 	}
 }

Signature · 3 lines

Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Bjorn Helgaas

2017-10-02 23:19 UTC #1 lore raw

Quoted 14 lines · Tyler Baicar

Correctable errors do not need any software intervention, so avoid calling into the software recovery process for correctable errors.
Signed-off-by Tyler Baicar <[email protected]>
---
 drivers/pci/pcie/aer/aerdrv_core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/pci/pcie/aer/aerdrv_core.c +0 -0
index b1303b3..4765c11 100644

drivers/pci/pcie/aer/aerdrv_core.c static void aer_recover_work_func(struct work_struct *work) · line 630

Quoted 6 lines

drivers/pci/pcie/aer/aerdrv_core.c +2 -1

@@ -626,7 +626,8 @@ static void aer_recover_work_func(struct work_struct *work)
 			continue;
 		}
 		cper_print_aer(pdev, entry.severity, entry.regs);
-		do_recovery(pdev, entry.severity);
+		if (entry.severity != AER_CORRECTABLE)
+			do_recovery(pdev, entry.severity);

I think this is fine, and it mirrors what is done in handle_error_source().

But I want to converge the APEI path and the "native" AER path, so as one tiny step in that direction, can you look into doing this test once, e.g., move the test from handle_error_source() into do_recovery(), where one test will handle both paths?

Quoted 8 lines

drivers/pci/pcie/aer/aerdrv_core.c +0 -0

 		pci_dev_put(pdev);
 	}
 }

Signature · 3 lines

Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Tyler Baicar

author

2017-10-11 14:37 UTC #2 lore raw

Quoted 24 lines · Bjorn Helgaas

Quoted 14 lines · Tyler Baicar
Correctable errors do not need any software intervention, so avoid calling into the software recovery process for correctable errors.
Signed-off-by Tyler Baicar <[email protected]>
---
  drivers/pci/pcie/aer/aerdrv_core.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)
drivers/pci/pcie/aer/aerdrv_core.c +0 -0
index b1303b3..4765c11 100644
drivers/pci/pcie/aer/aerdrv_core.c static void aer_recover_work_func(struct work_struct *work) · line 630
Quoted 6 lines
drivers/pci/pcie/aer/aerdrv_core.c +2 -1
@@ -626,7 +626,8 @@ static void aer_recover_work_func(struct work_struct *work)
  			continue;
  		}
  		cper_print_aer(pdev, entry.severity, entry.regs);
-		do_recovery(pdev, entry.severity);
+		if (entry.severity != AER_CORRECTABLE)
+			do_recovery(pdev, entry.severity);
I think this is fine, and it mirrors what is done in handle_error_source().

Quoted 4 lines

But I want to converge the APEI path and the "native" AER path, so as one tiny step in that direction, can you look into doing this test once, e.g., move the test from handle_error_source() into do_recovery(), where one test will handle both paths?

Hello Bjorn,

Thank you for the input!

I've looked into this and it seems there is still going to need to be two versions of this check. The native AER path goes through handle_error_source() and for correctable errors requires the write to PCI_ERR_COR_STATUS, but the APEI path does not require this write. I could move this check to the beginning of do_recovery() so it returns right away for correctable errors and remove the else line in handle_error_source() so it always calls into do_recovery(). That doesn't seem like a very clean solution though since then there are still two checks for correctable errors and now we're calling into do_recovery() in both cases for nothing :)

Thanks,
Tyler

Bjorn Helgaas

2017-10-11 17:09 UTC #3 lore raw

Quoted 30 lines · Tyler Baicar

Quoted 24 lines · Bjorn Helgaas
Quoted 14 lines · Tyler Baicar
Correctable errors do not need any software intervention, so avoid calling into the software recovery process for correctable errors.
Signed-off-by Tyler Baicar <[email protected]>
---
 drivers/pci/pcie/aer/aerdrv_core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/pci/pcie/aer/aerdrv_core.c +0 -0
index b1303b3..4765c11 100644
drivers/pci/pcie/aer/aerdrv_core.c static void aer_recover_work_func(struct work_struct *work) · line 630
Quoted 6 lines
drivers/pci/pcie/aer/aerdrv_core.c +2 -1
@@ -626,7 +626,8 @@ static void aer_recover_work_func(struct work_struct *work)
 			continue;
 		}
 		cper_print_aer(pdev, entry.severity, entry.regs);
-		do_recovery(pdev, entry.severity);
+		if (entry.severity != AER_CORRECTABLE)
+			do_recovery(pdev, entry.severity);
I think this is fine, and it mirrors what is done in handle_error_source().
Quoted 4 lines
But I want to converge the APEI path and the "native" AER path, so as one tiny step in that direction, can you look into doing this test once, e.g., move the test from handle_error_source() into do_recovery(), where one test will handle both paths?

Quoted 10 lines

I've looked into this and it seems there is still going to need to be two versions of this check. The native AER path goes through handle_error_source() and for correctable errors requires the write to PCI_ERR_COR_STATUS, but the APEI path does not require this write. I could move this check to the beginning of do_recovery() so it returns right away for correctable errors and remove the else line in handle_error_source() so it always calls into do_recovery(). That doesn't seem like a very clean solution though since then there are still two checks for correctable errors and now we're calling into do_recovery() in both cases for nothing :)

The PCI_ERR_COR_STATUS thing is part of what I see as the problem here. IMHO, the native AER path should collect up the log registers (and do any acknowledgement, e.g., writing PCI_ERR_COR_STATUS) *before* entering the common path.

In other words, the Linux code in the native part of AER should be functionally the same as the BIOS code that implements the APEI path.

This is a bit of restructuring in the Linux AER code. I haven't looked enough to know how much. If it's impractical, it's impractical. I thought this might be an opportunity for a tiny step in that direction, but if it's not, I guess that's OK.

Bjorn

Tyler Baicar

author

2017-11-07 23:27 UTC #4 lore raw

Quoted 45 lines · Bjorn Helgaas

Quoted 29 lines · Tyler Baicar
Quoted 24 lines · Bjorn Helgaas
Quoted 14 lines · Tyler Baicar
Correctable errors do not need any software intervention, so avoid calling into the software recovery process for correctable errors.
Signed-off-by Tyler Baicar <[email protected]>
---
  drivers/pci/pcie/aer/aerdrv_core.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)
drivers/pci/pcie/aer/aerdrv_core.c +0 -0
index b1303b3..4765c11 100644
drivers/pci/pcie/aer/aerdrv_core.c static void aer_recover_work_func(struct work_struct *work) · line 630
Quoted 6 lines
drivers/pci/pcie/aer/aerdrv_core.c +2 -1
@@ -626,7 +626,8 @@ static void aer_recover_work_func(struct work_struct *work)
  			continue;
  		}
  		cper_print_aer(pdev, entry.severity, entry.regs);
-		do_recovery(pdev, entry.severity);
+		if (entry.severity != AER_CORRECTABLE)
+			do_recovery(pdev, entry.severity);
I think this is fine, and it mirrors what is done in handle_error_source().
Quoted 4 lines
But I want to converge the APEI path and the "native" AER path, so as one tiny step in that direction, can you look into doing this test once, e.g., move the test from handle_error_source() into do_recovery(), where one test will handle both paths?
Quoted 10 lines
I've looked into this and it seems there is still going to need to be two versions of this check. The native AER path goes through handle_error_source() and for correctable errors requires the write to PCI_ERR_COR_STATUS, but the APEI path does not require this write. I could move this check to the beginning of do_recovery() so it returns right away for correctable errors and remove the else line in handle_error_source() so it always calls into do_recovery(). That doesn't seem like a very clean solution though since then there are still two checks for correctable errors and now we're calling into do_recovery() in both cases for nothing :)
The PCI_ERR_COR_STATUS thing is part of what I see as the problem here. IMHO, the native AER path should collect up the log registers (and do any acknowledgement, e.g., writing PCI_ERR_COR_STATUS) *before* entering the common path.

Quoted 7 lines

In other words, the Linux code in the native part of AER should be functionally the same as the BIOS code that implements the APEI path.
This is a bit of restructuring in the Linux AER code. I haven't looked enough to know how much. If it's impractical, it's impractical. I thought this might be an opportunity for a tiny step in that direction, but if it's not, I guess that's OK.

Hello Bjorn,

That restructuring doesn't look trivial to do in this patch, so do you think this patch is good for 4.15?

Thanks,
Tyler

Signature · 3 lines

Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Tyler Baicar

author

2017-11-15 14:46 UTC #5 lore raw

In reply to Bjorn Helgaas

Quoted 10 lines · Bjorn Helgaas

Quoted 9 lines · Tyler Baicar
Correctable errors do not need any software intervention, so avoid calling into the software recovery process for correctable errors.
Signed-off-by Tyler Baicar <[email protected]>
---
  drivers/pci/pcie/aer/aerdrv_core.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

Quoted 14 lines

drivers/pci/pcie/aer/aerdrv_core.c static void aer_recover_work_func(struct work_struct *work) · line 630
Quoted 11 lines
drivers/pci/pcie/aer/aerdrv_core.c +2 -1
index b1303b3..4765c11 100644
@@ -626,7 +626,8 @@ static void aer_recover_work_func(struct work_struct *work)
  			continue;
  		}
  		cper_print_aer(pdev, entry.severity, entry.regs);
-		do_recovery(pdev, entry.severity);
+		if (entry.severity != AER_CORRECTABLE)
+			do_recovery(pdev, entry.severity);
I think this is fine, and it mirrors what is done in handle_error_source().

Hello,

Will this patch be pulled into 4.15?

Thanks,
Tyler

Signature · 3 lines

Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Bjorn Helgaas

2017-11-17 18:23 UTC #6 lore raw

Quoted 25 lines · Tyler Baicar

Quoted 10 lines · Bjorn Helgaas
Quoted 9 lines · Tyler Baicar
Correctable errors do not need any software intervention, so avoid calling into the software recovery process for correctable errors.
Signed-off-by Tyler Baicar <[email protected]>
---
 drivers/pci/pcie/aer/aerdrv_core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Quoted 14 lines
drivers/pci/pcie/aer/aerdrv_core.c static void aer_recover_work_func(struct work_struct *work) · line 630
Quoted 11 lines
drivers/pci/pcie/aer/aerdrv_core.c +2 -1
index b1303b3..4765c11 100644
@@ -626,7 +626,8 @@ static void aer_recover_work_func(struct work_struct *work)
 			continue;
 		}
 		cper_print_aer(pdev, entry.severity, entry.regs);
-		do_recovery(pdev, entry.severity);
+		if (entry.severity != AER_CORRECTABLE)
+			do_recovery(pdev, entry.severity);
I think this is fine, and it mirrors what is done in handle_error_source().

Quoted 3 lines

Hello,
Will this patch be pulled into 4.15?

Sorry, I didn't get to this in time for v4.15, but I put it on pci/aer for v4.16. I expanded the changelog to note that this means we won't call the driver's callbacks or emit the "recovery successful" message for correctable errors from APEI, and that this matches what we already do for the non-APEI path.

Bjorn

commit 72761170e6d5519c91136fd6cc80805a74ef9cfd

Author: Tyler Baicar <[email protected]>
Date:   Mon Aug 28 11:09:44 2017 -0600

PCI/AER: Skip recovery callbacks for correctable errors from ACPI APEI

PCIe correctable errors are corrected by hardware. Software may log them, but no other software intervention is required.

There are two paths to enter the AER recovery code: (1) the native path where Linux fields the AER interrupt and reads the AER registers directly, and (2) the ACPI path where firmware reads the AER registers and hands them off to Linux via the ACPI APEI path.

The AER do_recovery() function calls driver error reporting callbacks (error_detected(), mmio_enabled(), resume(), etc), attempts recovery (for fatal errors), and logs a "AER: Device recovery successful" message.

Since there's nothing to recover for correctable errors, the native path already skips do_recovery(), so it doesn't call the driver callbacks and or emit the message. Make the APEI path do the same.

Signed-off-by Tyler Baicar <[email protected]>

[bhelgaas: changelog]

Signed-off-by Bjorn Helgaas <[email protected]>

drivers/pci/pcie/aer/aerdrv_core.c +2 -1

index 744805232155..3e354f224422 100644
@@ -633,7 +633,8 @@ static void aer_recover_work_func(struct work_struct *work)
 			continue;
 		}
 		cper_print_aer(pdev, entry.severity, entry.regs);
-		do_recovery(pdev, entry.severity);
+		if (entry.severity != AER_CORRECTABLE)
+			do_recovery(pdev, entry.severity);
 		pci_dev_put(pdev);
 	}
 }