Fix issues and cleanup for ERR_FATAL and ERR_NONFATAL

pci_cleanup_aer_uncorrect_error_status() is called by different slot_reset callbacks in case of ERR_NONFATAL.

AER uncorrectable error status should take severity into account in order to clear the bits, so that ERR_NONFATAL path does not clear the bit which are marked with severity fatal.

Signed-off-by Oza Pawandeep <[email protected]>

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

index a2e8838..d6cb1f0 100644
@@ -360,13 +360,16 @@ EXPORT_SYMBOL_GPL(pci_disable_pcie_error_reporting);
 int pci_cleanup_aer_uncorrect_error_status(struct pci_dev *dev)

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

 	pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_STATUS, &status);
+	pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_SEVER, &mask);
+	status &= ~mask; /* Clear corresponding nonfatal bits */
 	if (status)
 		pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_STATUS, status);
 

@@ -1336,8 +1339,6 @@ static pci_ers_result_t aer_root_reset(struct pci_dev *dev)
  */
 static void aer_error_resume(struct pci_dev *dev)
 {
-	int pos;
-	u32 status, mask;
 	u16 reg16;
 
 	/* Clean up Root device status */

@@ -1345,11 +1346,7 @@ static void aer_error_resume(struct pci_dev *dev)
 	pcie_capability_write_word(dev, PCI_EXP_DEVSTA, reg16);
 
 	/* Clean AER Root Error Status */
-	pos = dev->aer_cap;
-	pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_STATUS, &status);
-	pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_SEVER, &mask);
-	status &= ~mask; /* Clear corresponding nonfatal bits */
-	pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_STATUS, status);
+	pci_cleanup_aer_uncorrect_error_status(dev);
 }
 
 static struct pcie_port_service_driver aerdriver = {

Signature · 1 line

2.7.4

Hi Oza,

Thanks for doing this!

Quoted 2 lines · Oza Pawandeep

pci_cleanup_aer_uncorrect_error_status() is called by different slot_reset callbacks in case of ERR_NONFATAL.

IIRC, the current strategy is:

ERR_COR: log only
ERR_NONFATAL: call driver callbacks (pci_error_handlers)
ERR_FATAL: remove device and re-enumerate

So these slot_reset callbacks are only used for ERR_NONFATAL, which are all uncorrectable errors, of course.

This patch makes it so that when the slot_reset callbacks call pci_cleanup_aer_uncorrect_error_status(), we only clear the bits set by ERR_NONFATAL events (this is determined by PCI_ERR_UNCOR_SEVER).

That makes good sense to me. All these status bits are RW1CS, so they will be preserved across a reset but will be cleared when we re-enumerate, in this path:

pci_init_capabilities
pci_aer_init
pci_cleanup_aer_error_status_regs
# clear all PCI_ERR_UNCOR_STATUS and PCI_ERR_COR_STATUS bits

Quoted 3 lines

AER uncorrectable error status should take severity into account in order to clear the bits, so that ERR_NONFATAL path does not clear the bit which are marked with severity fatal.

Two comments:

1) Can you split this into two patches, one that changes pci_cleanup_aer_uncorrect_error_status() so it looks like the error clearing code in aer_error_resume(), and a second that factors out the duplicate code?

2) Maybe use "sev" or "sever" instead of "mask" for the local variable, since there is also a separate PCI_ERR_UNCOR_MASK register, which is not involved here.

3) The "pci_cleanup_aer_uncorrect_error_status()" name no longer really describes what this does. Something like "pci_aer_clear_nonfatal_status()" would be more descriptive. But I see you have a subsequent patch (which I haven't looked at yet) that is related to this.

4) I don't think the driver slot_reset callbacks should be responsible for clearing these AER status bits. Can we clear them somewhere in the pcie_do_nonfatal_recovery() path and remove these calls from the drivers?

5) In principle, we should only read PCI_ERR_UNCOR_STATUS *once* per device when handling an error. We currently read it three times:

aer_isr
aer_isr_one_error
find_source_device
find_device_iter
is_error_source
read PCI_ERR_UNCOR_STATUS # 1
aer_process_err_devices
get_device_error_info(e_info->dev[i])
read PCI_ERR_UNCOR_STATUS # 2
handle_error_source
pcie_do_nonfatal_recovery
...
report_slot_reset
driver->err_handler->slot_reset
pci_cleanup_aer_uncorrect_error_status
read PCI_ERR_UNCOR_STATUS # 3

OK, that was more than two comments :)

Quoted 37 lines · Bjorn Helgaas

Hi Oza,

Thanks for doing this!

Quoted 2 lines · Oza Pawandeep

pci_cleanup_aer_uncorrect_error_status() is called by different slot_reset callbacks in case of ERR_NONFATAL.

IIRC, the current strategy is:

ERR_COR: log only
ERR_NONFATAL: call driver callbacks (pci_error_handlers)
ERR_FATAL: remove device and re-enumerate

So these slot_reset callbacks are only used for ERR_NONFATAL, which are all uncorrectable errors, of course.

This patch makes it so that when the slot_reset callbacks call pci_cleanup_aer_uncorrect_error_status(), we only clear the bits set by ERR_NONFATAL events (this is determined by PCI_ERR_UNCOR_SEVER).

That makes good sense to me. All these status bits are RW1CS, so they will be preserved across a reset but will be cleared when we re-enumerate, in this path:

pci_init_capabilities
pci_aer_init
pci_cleanup_aer_error_status_regs
# clear all PCI_ERR_UNCOR_STATUS and PCI_ERR_COR_STATUS bits

Quoted 3 lines

AER uncorrectable error status should take severity into account in order to clear the bits, so that ERR_NONFATAL path does not clear the bit which are marked with severity fatal.

Two comments:

Quoted 8 lines

1) Can you split this into two patches, one that changes pci_cleanup_aer_uncorrect_error_status() so it looks like the error clearing code in aer_error_resume(), and a second that factors out the duplicate code?

2) Maybe use "sev" or "sever" instead of "mask" for the local variable, since there is also a separate PCI_ERR_UNCOR_MASK register, which is not involved here.

Let me back up a little here: I'm not asking you to do the things below here. They're just possible future things, so we can think about them after this series. And the things above are things I can easily do myself. So no action required from you, unless you think I'm on the wrong track :)

Quoted 33 lines

3) The "pci_cleanup_aer_uncorrect_error_status()" name no longer really describes what this does. Something like "pci_aer_clear_nonfatal_status()" would be more descriptive. But I see you have a subsequent patch (which I haven't looked at yet) that is related to this.

4) I don't think the driver slot_reset callbacks should be responsible for clearing these AER status bits. Can we clear them somewhere in the pcie_do_nonfatal_recovery() path and remove these calls from the drivers?

5) In principle, we should only read PCI_ERR_UNCOR_STATUS *once* per device when handling an error. We currently read it three times:

aer_isr
aer_isr_one_error
find_source_device
find_device_iter
is_error_source
read PCI_ERR_UNCOR_STATUS # 1
aer_process_err_devices
get_device_error_info(e_info->dev[i])
read PCI_ERR_UNCOR_STATUS # 2
handle_error_source
pcie_do_nonfatal_recovery
...
report_slot_reset
driver->err_handler->slot_reset
pci_cleanup_aer_uncorrect_error_status
read PCI_ERR_UNCOR_STATUS # 3

OK, that was more than two comments :)

We are handling ERR_FATAL by resetting the Link in software,skipping the driver pci_error_handlers callbacks, removing the devices from the PCI subsystem, and re-enumerating, because of, no need to handle pci_channel_io_frozen case anymore.

Besides the walk on the bus is happening on subordinates, inside broadcast_error_message(), which means that pcie_portdrv_slot_reset() is never called for RP, and now since the all the devices are removed under this downstream link, we can safely get rid of ERR_FATAL handling code in pcie_portdrv_slot_reset().

Signed-off-by Oza Pawandeep <[email protected]>

drivers/pci/pcie/portdrv_pci.c +0 -18

index 973f1b8..b970a6d 100644
@@ -42,17 +42,6 @@ __setup("pcie_ports=", pcie_port_setup);
 
 /* global data */
 
-static int pcie_portdrv_restore_config(struct pci_dev *dev)
-{
-	int retval;
-
-	retval = pci_enable_device(dev);
-	if (retval)
-		return retval;
-	pci_set_master(dev);
-	return 0;
-}
-
 #ifdef CONFIG_PM
 static int pcie_port_runtime_suspend(struct device *dev)
 {

@@ -163,13 +152,6 @@ static pci_ers_result_t pcie_portdrv_mmio_enabled(struct pci_dev *dev)
 static pci_ers_result_t pcie_portdrv_slot_reset(struct pci_dev *dev)
 {
 	/* If fatal, restore cfg space for possible link reset at upstream */
-	if (dev->error_state == pci_channel_io_frozen) {
-		dev->state_saved = true;
-		pci_restore_state(dev);
-		pcie_portdrv_restore_config(dev);
-		pci_enable_pcie_error_reporting(dev);
-	}
-
 	return PCI_ERS_RESULT_RECOVERED;
 }
 

Signature · 1 line

2.7.4

These are follow up patches for the series which modifies ERR_FATAL handling. although there were couple of problems existed before which, itis also fixing.

patch-1:
Fixes the problem where ERR_FATAL and ERR_NONFATAL status should be cleared taking severity mask into account.

patch-2:
Takes care of clearing error fatal status

patch-3:
Follow up patch where no more need of handling ERR_FATAL case.

patch-4:
Fixes clearing device status in case of uncorrectable errors. (e.g. ERR_FATAL and ERR_NONFATAL)

patch-5:
Fixes clearing device status in case of correctable errors.

patch-6:
Follow up patch where no more need of handling pci_channel_io_frozen in pcie_portdrv_slot_reset()

Oza Pawandeep (6):
PCI/AER: Take severity mask into account while clearing error bits PCI/AER: Clear uncorrectable fatal error status bits PCI/ERR: Cleanup ERR_FATAL of error broadcast
PCI/AER: Clear device error status bits during ERR_FATAL and
ERR_NONFATAL
PCI/AER: Fix correctable status bits clearing in device register PCI/PORTDRV: Remove ERR_FATAL handling from pcie_portdrv_slot_reset()

 drivers/pci/pcie/aer.c         | 35 
+++++++++++++++++++++++------------
 drivers/pci/pcie/err.c         | 15 +++++++--------
 drivers/pci/pcie/portdrv_pci.c | 18 ------------------
 include/linux/aer.h            |  5 +++++
 4 files changed, 35 insertions(+), 38 deletions(-)