PCI/AER: Take mask into account while clearing error bits

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, as a result of that, no more calling pcie_portdrv_slot_reset in ERR_FATAL case.

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

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

index 973f1b8..92f5d330 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)
 {

@@ -162,14 +151,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)
 {

Quoted 35 lines · Oza Pawandeep

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, as a result of that, no more calling pcie_portdrv_slot_reset in ERR_FATAL case.

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

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

index 973f1b8..92f5d330 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)
 {

@@ -162,14 +151,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)
 {

drivers/pci/pcie/portdrv_pci.c static pci_ers_result_t · line 156

Quoted 10 lines

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

-	/* 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;
 }

Hi Bjorn,

the above patch removes ERR_FATAL handling from pcie_portdrv_slot_reset() because now we are handling ERR_FATAL differently than before.

I tried to dig into pcie_portdrv_slot_reset() handling for ERR_FATAL case where it
restores the config space, enable device, set master and enable error reporting....
and as far as I understand this is being done for upstream link (bridges etc..)

why was it done at the first point (I checked the commit description, but could not really get it)
and do we need to handle the same thing in ERR_FATAL now ?

Quoted 60 lines · On Thu, Jun 07, 2018 at 07:18:03PM +0530, [email protected] wrote:

Quoted 35 lines · Oza Pawandeep

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, as a result of that, no more calling pcie_portdrv_slot_reset in ERR_FATAL case.

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

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

index 973f1b8..92f5d330 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)
 {

@@ -162,14 +151,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)
 {

drivers/pci/pcie/portdrv_pci.c static pci_ers_result_t · line 156

Quoted 10 lines

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

-	/* 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;
 }

Hi Bjorn,

the above patch removes ERR_FATAL handling from pcie_portdrv_slot_reset() because now we are handling ERR_FATAL differently than before.

I tried to dig into pcie_portdrv_slot_reset() handling for ERR_FATAL case where it
restores the config space, enable device, set master and enable error reporting....
and as far as I understand this is being done for upstream link (bridges etc..)

Quoted 3 lines

why was it done at the first point (I checked the commit description, but could not really get it)
and do we need to handle the same thing in ERR_FATAL now ?

You mean 4bf3392e0bf5 ("PCI-Express AER implemetation: pcie_portdrv error handler"), which added pcie_portdrv_slot_reset()? I agree, that commit log has no useful information. I don't know any of the history behind it.

Keith,

do you know why in ERR_FATAL case following was done ?
have a look at pcie_portdrv_slot_reset() handling (for bridges, switches etc..)

Quoted 60 lines · On Thu, Jun 07, 2018 at 07:18:03PM +0530, [email protected] wrote:

Quoted 35 lines · Oza Pawandeep

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, as a result of that, no more calling pcie_portdrv_slot_reset in ERR_FATAL case.

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

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

index 973f1b8..92f5d330 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)
 {

@@ -162,14 +151,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)
 {

drivers/pci/pcie/portdrv_pci.c static pci_ers_result_t · line 156

Quoted 10 lines

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

-	/* 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;
 }

Hi Bjorn,

the above patch removes ERR_FATAL handling from pcie_portdrv_slot_reset() because now we are handling ERR_FATAL differently than before.

I tried to dig into pcie_portdrv_slot_reset() handling for ERR_FATAL case where it
restores the config space, enable device, set master and enable error reporting....
and as far as I understand this is being done for upstream link (bridges etc..)

Quoted 3 lines

why was it done at the first point (I checked the commit description, but could not really get it)
and do we need to handle the same thing in ERR_FATAL now ?

You mean 4bf3392e0bf5 ("PCI-Express AER implemetation: pcie_portdrv error handler"), which added pcie_portdrv_slot_reset()? I agree, that commit log has no useful information. I don't know any of the history behind it.

Quoted 61 lines · Bjorn Helgaas

Quoted 60 lines · On Thu, Jun 07, 2018 at 07:18:03PM +0530, [email protected] wrote:

Quoted 35 lines · Oza Pawandeep

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, as a result of that, no more calling pcie_portdrv_slot_reset in ERR_FATAL case.

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

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

index 973f1b8..92f5d330 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)
 {

@@ -162,14 +151,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)
 {

drivers/pci/pcie/portdrv_pci.c static pci_ers_result_t · line 156

Quoted 10 lines

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

-	/* 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;
 }

Hi Bjorn,

the above patch removes ERR_FATAL handling from pcie_portdrv_slot_reset() because now we are handling ERR_FATAL differently than before.

I tried to dig into pcie_portdrv_slot_reset() handling for ERR_FATAL case where it
restores the config space, enable device, set master and enable error reporting....
and as far as I understand this is being done for upstream link (bridges etc..)

Quoted 8 lines

Quoted 3 lines

why was it done at the first point (I checked the commit description, but could not really get it)
and do we need to handle the same thing in ERR_FATAL now ?

You mean 4bf3392e0bf5 ("PCI-Express AER implemetation: pcie_portdrv error handler"), which added pcie_portdrv_slot_reset()? I agree, that commit log has no useful information. I don't know any of the history behind it.

Yes Bjorn thats right.
I am trying to understand it but no clue.
since it is restoring the stuffs in ERR_FATAL case, why would PCIe bridge loose all the settings ? [config space, aer bits, master, device enable etc..)
Max we do is link_reset in ERR_FATAL case, and Secondary bus reset should affect downstream components (not upstream)

Quoted 60 lines · On Thu, Jun 07, 2018 at 07:18:03PM +0530, [email protected] wrote:

Quoted 35 lines · Oza Pawandeep

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, as a result of that, no more calling pcie_portdrv_slot_reset in ERR_FATAL case.

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

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

index 973f1b8..92f5d330 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)
 {

@@ -162,14 +151,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)
 {

drivers/pci/pcie/portdrv_pci.c static pci_ers_result_t · line 156

Quoted 10 lines

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

-	/* 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;
 }

Hi Bjorn,

the above patch removes ERR_FATAL handling from pcie_portdrv_slot_reset() because now we are handling ERR_FATAL differently than before.

I tried to dig into pcie_portdrv_slot_reset() handling for ERR_FATAL case where it
restores the config space, enable device, set master and enable error reporting....
and as far as I understand this is being done for upstream link (bridges etc..)

Quoted 3 lines

why was it done at the first point (I checked the commit description, but could not really get it)
and do we need to handle the same thing in ERR_FATAL now ?

You mean 4bf3392e0bf5 ("PCI-Express AER implemetation: pcie_portdrv error handler"), which added pcie_portdrv_slot_reset()? I agree, that commit log has no useful information. I don't know any of the history behind it.

Quoted 61 lines · Bjorn Helgaas

Quoted 60 lines · On Thu, Jun 07, 2018 at 07:18:03PM +0530, [email protected] wrote:

Quoted 35 lines · Oza Pawandeep

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, as a result of that, no more calling pcie_portdrv_slot_reset in ERR_FATAL case.

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

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

index 973f1b8..92f5d330 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)
 {

@@ -162,14 +151,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)
 {

drivers/pci/pcie/portdrv_pci.c static pci_ers_result_t · line 156

Quoted 10 lines

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

-	/* 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;
 }

Hi Bjorn,

the above patch removes ERR_FATAL handling from pcie_portdrv_slot_reset() because now we are handling ERR_FATAL differently than before.

I tried to dig into pcie_portdrv_slot_reset() handling for ERR_FATAL case where it
restores the config space, enable device, set master and enable error reporting....
and as far as I understand this is being done for upstream link (bridges etc..)

Quoted 8 lines

Quoted 3 lines

why was it done at the first point (I checked the commit description, but could not really get it)
and do we need to handle the same thing in ERR_FATAL now ?

You mean 4bf3392e0bf5 ("PCI-Express AER implemetation: pcie_portdrv error handler"), which added pcie_portdrv_slot_reset()? I agree, that commit log has no useful information. I don't know any of the history behind it.

Hi Bjorn and Keith,

broadcast_error_message()

if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
        .....
        pci_walk_bus(dev->subordinate, cb, &result_data);

so in case of ERR_FATAL, the walk bus is happening on subordinates, and if I understand the walk right
then, pcie_portdrv_slot_reset() is called only on BRIDGES/Switches

If is never called on Root-Ports

having said that, now since we are removing the devices (compare to previous error callback handling in ERR_FATAL)
I dont see the need of the above code anymore.

because there is nothing to restore to any more. as we are initiating re-enumeration.

Regards,
Oza.

PCIe ERR_NONFATAL and ERR_FATAL are uncorrectable errors, and clearing uncorrectable error bits should take error mask into account.

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

drivers/pci/pcie/aer/aerdrv.c +1 -7

index 377e576..8cbc62b 100644
@@ -341,8 +341,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 */

@@ -350,11 +348,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);
 }
 
 /**

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

index 946f3f6..309f3f5 100644
@@ -50,13 +50,17 @@ EXPORT_SYMBOL_GPL(pci_disable_pcie_error_reporting);
 int pci_cleanup_aer_uncorrect_error_status(struct pci_dev *dev)
 {
 	int pos;
-	u32 status;
+	u32 status, mask;
 
 	pos = dev->aer_cap;
 	if (!pos)
 		return -EIO;
 
+	/* 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 */
 	if (status)
 		pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_STATUS, status);
 

Signature · 1 line

2.7.4

Quoted 4 lines · Oza Pawandeep

PCIe ERR_NONFATAL and ERR_FATAL are uncorrectable errors, and clearing uncorrectable error bits should take error mask into account.

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

If/when you repost these, please include a [0/6] cover letter with an overview of the purpose of the series.

I assume these are for v4.19, so I'll look at them after the merge window.

If they fix issues introduced during the v4.18 merge window, we may be able to merge them during the v4.18 -rc cycle. In this case, I would need specifics about what exactly the problems are.

Quoted 55 lines

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

index 377e576..8cbc62b 100644
@@ -341,8 +341,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;
 

drivers/pci/pcie/aer/aerdrv.c +1 -5

 	/* Clean up Root device status */

@@ -350,11 +348,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);
 }
 
 /**

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

index 946f3f6..309f3f5 100644
@@ -50,13 +50,17 @@ 
 EXPORT_SYMBOL_GPL(pci_disable_pcie_error_reporting);
 int pci_cleanup_aer_uncorrect_error_status(struct pci_dev *dev)
 {
 	int pos;
-	u32 status;
+	u32 status, mask;
 
 	pos = dev->aer_cap;
 	if (!pos)
 		return -EIO;
 
+	/* 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 */
 	if (status)
 		pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_STATUS, status);
 
--

2.7.4