PCI/AER: Return approrpiate value when AER is not supported

drivers/pci/pcie/pcie-dpc.c static int dpc_probe(struct pcie_device *dev) · line 295

Quoted 7 lines · Keith Busch

drivers/pci/pcie/pcie-dpc.c +3 -0

@@ -289,6 +290,9 @@ static int dpc_probe(struct pcie_device *dev)
 	int status;
 	u16 ctl, cap;
 
+	if (pcie_aer_get_firmware_first(pdev))
+		return -ENOTSUPP;
+

There are two ways to support firmware first handling along with DPC.

The first one is to tie DPC handling to the firmware first enable.

The second one is to enable DPC ERR_COR signalling so that firmware gets notified on each DPC event occurrence.

While the first one gives more control to the firmware, I think it beats the purpose of the DPC. The first approach requires firmware to do some "pre-processing" before notifying operating system of a failure.

The goal of the DPC is to put hardware into safe state when a PCIe error happens. The best software recovery following this is to notify endpoint drivers of failures and shutdown threads/processes accessing the hardware as quick as possible.

The firmware-first event notification is through ACPI GHES and firmware injects an artifical uncorrected AER error to the operating system. Once OS gets notified, it tries to recover drivers through AER fatal error recovery mechanism.

While the semantics of this path is clearly defined in ACPI, it is also known to be slow as well. During the time firmware does its business, operating system still could be trying to access the endpoint address space.

My suggestion is to enable ERR_COR signalling so firmware gets a notification on each DPC event for logging purposes.

OS handles DPC natively and tries to recover hardware without any external influence.

Quoted 36 lines · Sinan Kaya

drivers/pci/pcie/pcie-dpc.c static int dpc_probe(struct pcie_device *dev) · line 295

Quoted 7 lines · Keith Busch

drivers/pci/pcie/pcie-dpc.c +3 -0

@@ -289,6 +290,9 @@ static int dpc_probe(struct pcie_device *dev)
 	int status;
 	u16 ctl, cap;
 
+	if (pcie_aer_get_firmware_first(pdev))
+		return -ENOTSUPP;
+

There are two ways to support firmware first handling along with DPC.

The first one is to tie DPC handling to the firmware first enable.

The second one is to enable DPC ERR_COR signalling so that firmware gets notified on each DPC event occurrence.

While the first one gives more control to the firmware, I think it beats the purpose of the DPC. The first approach requires firmware to do some "pre-processing" before notifying operating system of a failure.

The goal of the DPC is to put hardware into safe state when a PCIe error happens. The best software recovery following this is to notify endpoint drivers of failures and shutdown threads/processes accessing the hardware as quick as possible.

The firmware-first event notification is through ACPI GHES and firmware injects an artifical uncorrected AER error to the operating system. Once OS gets notified, it tries to recover drivers through AER fatal error recovery mechanism.

While the semantics of this path is clearly defined in ACPI, it is also known to be slow as well. During the time firmware does its business, operating system still could be trying to access the endpoint address space.

My suggestion is to enable ERR_COR signalling so firmware gets a notification on each DPC event for logging purposes.

Quoted 2 lines

OS handles DPC natively and tries to recover hardware without any external influence.

I see what you're saying, but if a device has a firmware first policy, doesn't that mean firmware owns the DPC Control register? The OS shouldn't be mucking with it in that case, right?

A DPC enabled device will suppress sending ERR_FATAL and ERR_NONFATAL, which prevents the AER handler from reporting the details of the error. This patch will have the DPC driver get the AER status registers from the downstream port that detected the uncorrectable error, and print out additional information.

Signed-off-by Keith Busch <[email protected]>

---

+Oza

Quoted 6 lines · Keith Busch

A DPC enabled device will suppress sending ERR_FATAL and ERR_NONFATAL, which prevents the AER handler from reporting the details of the error. This patch will have the DPC driver get the AER status registers from the downstream port that detected the uncorrectable error, and print out additional information.

Quoted 2 lines

Signed-off-by Keith Busch <[email protected]>

---

Oza is doing some restructuring to unify DPC and AER error handling path per feedback from Bjorn. It is almost done. He is testing it.

Can this patch wait until you review his version? I'm thinking this could be something that can be added to his series instead.

Coming back to this patch. The interrupt number for DPC and AER could be the same or different.

According to the spec, AER errors are always reported regardless of DPC driver presence (see the famous flow chart).

If the interrupt IDs are the same for AER and DPC, your patch would introduce double printing for AER errors.

Quoted 9 lines · Sinan Kaya

+Oza

Quoted 6 lines · Keith Busch

A DPC enabled device will suppress sending ERR_FATAL and ERR_NONFATAL, which prevents the AER handler from reporting the details of the error. This patch will have the DPC driver get the AER status registers from the downstream port that detected the uncorrectable error, and print out additional information.

Quoted 8 lines

Quoted 2 lines

Signed-off-by Keith Busch <[email protected]>

---

Oza is doing some restructuring to unify DPC and AER error handling path per feedback from Bjorn. It is almost done. He is testing it.

Can this patch wait until you review his version? I'm thinking this could be something that can be added to his series instead.

No problem.

Quoted 2 lines

Coming back to this patch. The interrupt number for DPC and AER could be the same or different.

Only if you're talking about root ports. DPC is also a feature of switch downstream ports, which don't generate interrupts on AER events (they don't have a Root Error Command register).

Quoted 5 lines

According to the spec, AER errors are always reported regardless of DPC driver presence (see the famous flow chart).

If the interrupt IDs are the same for AER and DPC, your patch would introduce double printing for AER errors.

The AER Uncorrectable Status Register of the detecting port would indeed be set with the appropriate status if that's the type of error that occured, but when DPC is enabled, the root port never observes an ERR_FATAL/ERR_NONFATAL message required for it to get set the Root Error Status Register. The Linux AER handler requires the Root Error Status be set in order for it to print anything, so I don't think we're at risk of double printing with this patch even if the root port is DPC capable.

Since the AER status is set when we observe DPC event and nobody is clearing these we won't observe another DPC event until somebody clears these. We can say that we are resetting the endpoints as part of the DPC but we are not touching the switch downstream port or the root port registers.

Somebody still needs to clear these in addition to printing whatever information is available in the AER registers.

Do you agree?

Quoted 6 lines · Sinan Kaya

Since the AER status is set when we observe DPC event and nobody is clearing these we won't observe another DPC event until somebody clears these. We can say that we are resetting the endpoints as part of the DPC but we are not touching the switch downstream port or the root port registers.

Quoted 2 lines

Somebody still needs to clear these in addition to printing whatever information is available in the AER registers.

We should clear the downstream port's AER uncorrectable status register if the trigger reason is of that type, but DPC definitely does not require we clear it in order to observe a new event.

Quoted 10 lines · Keith Busch

Quoted 8 lines · Sinan Kaya

Since the AER status is set when we observe DPC event and nobody is clearing these we won't observe another DPC event until somebody clears these. We can say that we are resetting the endpoints as part of the DPC but we are not touching the switch downstream port or the root port registers.

Somebody still needs to clear these in addition to printing whatever information is available in the AER registers.

Quoted 3 lines

We should clear the downstream port's AER uncorrectable status register if the trigger reason is of that type, but DPC definitely does not require we clear it in order to observe a new event.

I don't quite understand this. We don't need to clear the AER status in order to observe a new DPC event, but I think we *do* need to clear the AER status in order to log future AER events. Don't we?

I think Sinan is saying that if a DPC Port observes its own unmasked uncorrectable error (i.e., not something it learned about by receiving an ERR_* message), it will set a bit in an AER status register.

Since we do not set DPC ERR_COR Enable, the DPC Port does not generate an ERR_COR Message, so the AER driver never learns about the error and never clears the AER status register. So we'll decode the AER status (with your current patch), but we don't clear it, so if another error occurs, the AER logging won't work correctly.

Maybe we should be setting DPC ERR_COR Enable and letting the AER driver decode and clear the AER info?

I think Sinan is saying that if a DPC Port observes its own unmasked uncorrectable error (i.e., not something it learned about by receiving an ERR_* message), it will set a bit in an AER status register.

Since we do not set DPC ERR_COR Enable, the DPC Port does not generate an ERR_COR Message, so the AER driver never learns about the error and never clears the AER status register. So we'll decode the AER status (with your current patch), but we don't clear it, so if another error occurs, the AER logging won't work correctly.

A DPC enabled device will suppress sending ERR_FATAL and ERR_NONFATAL, which prevents the AER handler from reporting the details of the error. This patch will have the DPC driver get the AER status registers from the downstream port that detected the uncorrectable error, and print out additional information.

Signed-off-by Keith Busch <[email protected]>

---
 drivers/pci/pcie/pcie-dpc.c | 8 ++++++++
 1 file changed, 8 insertions(+)

drivers/pci/pcie/pcie-dpc.c +1 -0

index ef71a472592c..aefc4fbdcef0 100644
@@ -44,6 +44,7 @@ struct dpc_dev {
 	int			cap_pos;
 	bool			rp;
 	u32			rp_pio_status;
+	struct aer_err_info	info;
 };
 
 static const char * const rp_pio_error_string[] = {

drivers/pci/pcie/pcie-dpc.c +4 -0

 	pci_lock_rescan_remove();
 	list_for_each_entry_safe_reverse(dev, temp, &parent->devices,
 					 bus_list) {

@@ -275,6 +279,10 @@ static irqreturn_t dpc_irq(int irq, void *context)
 		/* show RP PIO error detail information */
 		if (reason == 3 && ext_reason == 0)
 			dpc_process_rp_pio_error(dpc);
+		if (reason == 2)
+			dpc->info.severity = AER_FATAL;
+		else
+			dpc->info.severity = AER_NONFATAL;
 
 		schedule_work(&dpc->work);
 	}

Signature · 1 line

2.13.6

Hi Keith,

Quoted 24 lines · Keith Busch

A DPC enabled device will suppress sending ERR_FATAL and ERR_NONFATAL, which prevents the AER handler from reporting the details of the error. This patch will have the DPC driver get the AER status registers from the downstream port that detected the uncorrectable error, and print out additional information.

Signed-off-by Keith Busch <[email protected]>

---
 drivers/pci/pcie/pcie-dpc.c | 8 ++++++++
 1 file changed, 8 insertions(+)

drivers/pci/pcie/pcie-dpc.c +1 -0

index ef71a472592c..aefc4fbdcef0 100644
@@ -44,6 +44,7 @@ struct dpc_dev {
 	int			cap_pos;
 	bool			rp;
 	u32			rp_pio_status;
+	struct aer_err_info	info;
 };
 
 static const char * const rp_pio_error_string[] = {

drivers/pci/pcie/pcie-dpc.c static void interrupt_event_handler(struct work_struct *work) · line 116

Quoted 5 lines

drivers/pci/pcie/pcie-dpc.c +2 -0

@@ -111,6 +112,9 @@ static void interrupt_event_handler(struct work_struct *work)
 	struct pci_dev *dev, *temp, *pdev = dpc->dev->port;
 	struct pci_bus *parent = pdev->subordinate;
 
+	if (aer_get_device_error_info(pdev, &dpc->info))
+		aer_print_error(pdev, &dpc->info);

I'm confused about this. "pdev" is the DPC-enabled Port, i.e., the Port that received an ERR_FATAL or ERR_NONFATAL Message. The Message was generated by something below "pdev", and that's where the interesting AER logging would have been done.

This patch suggests that the DPC port itself would have something interesting logged in its AER capability. Is that really true? I see in sec 6.2.10 (PCIe r4.0) that the DPC port should log the Trigger Reason and the Error Source ID from the Message, but I don't see anything about logging anything in its AER registers.

Quoted 38 lines · Bjorn Helgaas

Hi Keith,

Quoted 24 lines · Keith Busch

A DPC enabled device will suppress sending ERR_FATAL and ERR_NONFATAL, which prevents the AER handler from reporting the details of the error. This patch will have the DPC driver get the AER status registers from the downstream port that detected the uncorrectable error, and print out additional information.

Signed-off-by Keith Busch <[email protected]>

---
 drivers/pci/pcie/pcie-dpc.c | 8 ++++++++
 1 file changed, 8 insertions(+)

drivers/pci/pcie/pcie-dpc.c +1 -0

index ef71a472592c..aefc4fbdcef0 100644
@@ -44,6 +44,7 @@ struct dpc_dev {
 	int			cap_pos;
 	bool			rp;
 	u32			rp_pio_status;
+	struct aer_err_info	info;
 };
 
 static const char * const rp_pio_error_string[] = {

drivers/pci/pcie/pcie-dpc.c static void interrupt_event_handler(struct work_struct *work) · line 116

Quoted 5 lines

drivers/pci/pcie/pcie-dpc.c +2 -0

@@ -111,6 +112,9 @@ static void interrupt_event_handler(struct work_struct *work)
 	struct pci_dev *dev, *temp, *pdev = dpc->dev->port;
 	struct pci_bus *parent = pdev->subordinate;
 
+	if (aer_get_device_error_info(pdev, &dpc->info))
+		aer_print_error(pdev, &dpc->info);

I'm confused about this. "pdev" is the DPC-enabled Port, i.e., the Port that received an ERR_FATAL or ERR_NONFATAL Message. The Message was generated by something below "pdev", and that's where the interesting AER logging would have been done.

Quoted 5 lines

This patch suggests that the DPC port itself would have something interesting logged in its AER capability. Is that really true? I see in sec 6.2.10 (PCIe r4.0) that the DPC port should log the Trigger Reason and the Error Source ID from the Message, but I don't see anything about logging anything in its AER registers.

If the trigger reason is "unmasked uncorrectable error", then the DPC AER register has useful information. All other reasons would not provide useful data in the AER register.

I would assume that aer_get_device_error_info would return false if it's one of other reasons, but I can fix this up to check the trigger reasoning rather than unconditionally reading the AER status

Quoted 45 lines · Keith Busch

Quoted 38 lines · Bjorn Helgaas

Hi Keith,

Quoted 24 lines · Keith Busch

A DPC enabled device will suppress sending ERR_FATAL and ERR_NONFATAL, which prevents the AER handler from reporting the details of the error. This patch will have the DPC driver get the AER status registers from the downstream port that detected the uncorrectable error, and print out additional information.

Signed-off-by Keith Busch <[email protected]>

---
 drivers/pci/pcie/pcie-dpc.c | 8 ++++++++
 1 file changed, 8 insertions(+)

drivers/pci/pcie/pcie-dpc.c +1 -0

index ef71a472592c..aefc4fbdcef0 100644
@@ -44,6 +44,7 @@ struct dpc_dev {
 	int			cap_pos;
 	bool			rp;
 	u32			rp_pio_status;
+	struct aer_err_info	info;
 };
 
 static const char * const rp_pio_error_string[] = {

drivers/pci/pcie/pcie-dpc.c static void interrupt_event_handler(struct work_struct *work) · line 116

Quoted 5 lines

drivers/pci/pcie/pcie-dpc.c +2 -0

@@ -111,6 +112,9 @@ static void interrupt_event_handler(struct work_struct *work)
 	struct pci_dev *dev, *temp, *pdev = dpc->dev->port;
 	struct pci_bus *parent = pdev->subordinate;
 
+	if (aer_get_device_error_info(pdev, &dpc->info))
+		aer_print_error(pdev, &dpc->info);

I'm confused about this. "pdev" is the DPC-enabled Port, i.e., the Port that received an ERR_FATAL or ERR_NONFATAL Message. The Message was generated by something below "pdev", and that's where the interesting AER logging would have been done.

Quoted 5 lines

This patch suggests that the DPC port itself would have something interesting logged in its AER capability. Is that really true? I see in sec 6.2.10 (PCIe r4.0) that the DPC port should log the Trigger Reason and the Error Source ID from the Message, but I don't see anything about logging anything in its AER registers.

Quoted 9 lines

If the trigger reason is "unmasked uncorrectable error", then the DPC AER register has useful information. All other reasons would not provide useful data in the AER register.

I would assume that aer_get_device_error_info would return false if it's one of other reasons, but I can fix this up to check the trigger reasoning rather than unconditionally reading the AER status

https://pcisig.com/sites/default/files/specification_documents/ECN_DPC_2012-02-09_finalized.pdf

If you look at figure 6-2, AER errors are always logged regardless of the DPC presence.

Quoted 38 lines · Bjorn Helgaas

Hi Keith,

Quoted 24 lines · Keith Busch

A DPC enabled device will suppress sending ERR_FATAL and ERR_NONFATAL, which prevents the AER handler from reporting the details of the error. This patch will have the DPC driver get the AER status registers from the downstream port that detected the uncorrectable error, and print out additional information.

Signed-off-by Keith Busch <[email protected]>

---
 drivers/pci/pcie/pcie-dpc.c | 8 ++++++++
 1 file changed, 8 insertions(+)

drivers/pci/pcie/pcie-dpc.c +1 -0

index ef71a472592c..aefc4fbdcef0 100644
@@ -44,6 +44,7 @@ struct dpc_dev {
 	int			cap_pos;
 	bool			rp;
 	u32			rp_pio_status;
+	struct aer_err_info	info;
 };
 
 static const char * const rp_pio_error_string[] = {

drivers/pci/pcie/pcie-dpc.c static void interrupt_event_handler(struct work_struct *work) · line 116

Quoted 5 lines

drivers/pci/pcie/pcie-dpc.c +2 -0

@@ -111,6 +112,9 @@ static void interrupt_event_handler(struct work_struct *work)
 	struct pci_dev *dev, *temp, *pdev = dpc->dev->port;
 	struct pci_bus *parent = pdev->subordinate;
 
+	if (aer_get_device_error_info(pdev, &dpc->info))
+		aer_print_error(pdev, &dpc->info);

I'm confused about this. "pdev" is the DPC-enabled Port, i.e., the Port that received an ERR_FATAL or ERR_NONFATAL Message. The Message was generated by something below "pdev", and that's where the interesting AER logging would have been done.

Quoted 5 lines

This patch suggests that the DPC port itself would have something interesting logged in its AER capability. Is that really true? I see in sec 6.2.10 (PCIe r4.0) that the DPC port should log the Trigger Reason and the Error Source ID from the Message, but I don't see anything about logging anything in its AER registers.

If the trigger reason is "unmasked uncorrectable error", then the DPC AER register has useful information. All other reasons would not provide useful data in the AER register.

I would assume that aer_get_device_error_info would return false if it's one of other reasons, but I can fix this up to check the trigger reasoning rather than unconditionally reading the AER status

Getting the AER information is documented to return 0 when it failed to get the information.

Signed-off-by Keith Busch <[email protected]>

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

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

index 744805232155..ea0dc1cc7fc7 100644
@@ -660,7 +660,7 @@ static int get_device_error_info(struct pci_dev *dev, struct aer_err_info *info)
 
 	/* The device might not support AER */
 	if (!pos)
-		return 1;
+		return 0;
 
 	if (info->severity == AER_CORRECTABLE) {
 		pci_read_config_dword(dev, pos + PCI_ERR_COR_STATUS,

Signature · 1 line

2.13.6

s/approrpiate/appropriate/

(But maybe should be restated altogether, see below.)

Quoted 2 lines · Keith Busch

Getting the AER information is documented to return 0 when it failed to get the information.

I think this case is either impossible (if we only call this function for devices known to support AER), or it fixes an actual bug (the caller would call aer_print_error() when it shouldn't, and potentially print garbage). Right?

If the former, I vote for removing the test. If the latter, the changelog should mention that it fixes a bug.

Quoted 19 lines

Signed-off-by Keith Busch <[email protected]>

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

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

index 744805232155..ea0dc1cc7fc7 100644
@@ -660,7 +660,7 @@ static int get_device_error_info(struct pci_dev *dev, struct aer_err_info *info)
 
 	/* The device might not support AER */
 	if (!pos)
-		return 1;
+		return 0;
 
 	if (info->severity == AER_CORRECTABLE) {
 		pci_read_config_dword(dev, pos + PCI_ERR_COR_STATUS,
--

Quoted 2 lines

2.13.6

.

Quoted 2 lines · Keith Busch

Getting the AER information is documented to return 0 when it failed to get the information.

I think this case is either impossible (if we only call this function for devices known to support AER), or it fixes an actual bug (the caller would call aer_print_error() when it shouldn't, and potentially print garbage). Right?

If the former, I vote for removing the test. If the latter, the changelog should mention that it fixes a bug.

Getting the AER information is documented to return 0 when it failed to get the information.

Signed-off-by Keith Busch <[email protected]>

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

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

index 744805232155..ea0dc1cc7fc7 100644
@@ -660,7 +660,7 @@ static int get_device_error_info(struct pci_dev *dev, struct aer_err_info *info)
 
 	/* The device might not support AER */
 	if (!pos)
-		return 1;
+		return 0;
 
 	if (info->severity == AER_CORRECTABLE) {
 		pci_read_config_dword(dev, pos + PCI_ERR_COR_STATUS,

Signature · 1 line

2.13.6

Quoted 3 lines · Keith Busch

Getting the AER information is documented to return 0 when it failed to get the information.

Quoted 1 line

Signed-off-by Keith Busch <[email protected]>

Keith, I'm sorry, but I lost track of where we're at with this series. Can you repost a fresh version if it's still applicable?

Don't worry about rebasing it to anything other than v4.16-rc1 (my "master" branch). I might have added some conflicting things, but I'll take care of resolving any conflicts.