PCI/AER: Do not clear AER bits if we don't own AER

Quoted 2 lines · Alexandru Gagniuc

+    if (pcie_aer_get_firmware_first(dev))
+        return -EIO;

Can you move this to closer to the caller pci_aer_init()?

I find the intent clearer if we check it here rather than having to do the mental parsing of the state of aer_cap.

Quoted 1 line · Alex G.

I find the intent clearer if we check it here rather than having to do the mental parsing of the state of aer_cap.

I don't feel too strong about my comment to be honest. This was a style/maintenance comment.

It feels like we are putting pcie_aer_get_firmware_first() into core functions unnecessarily after your change. I understand the need for your change. I'm asking if it is the right place or not.

pcie_aer_get_firmware_first() should be called from either the init or probe function so that the rest of the AER functions do not get called from any other context.

If someone adds another AER function, we might need to add another pcie_aer_get_firmware_first() check there. So, we have unnecessary code duplication.

When we don't own AER, we shouldn't touch the AER error bits. Clearing error bits willy-nilly might cause firmware to miss some errors. In theory, these bits get cleared by FFS, or via ACPI _HPX method. These mechanisms are not subject to the problem.

This race is mostly of theoretical significance, since I can't reasonably demonstrate this race in the lab.

On a side-note, pcie_aer_is_kernel_first() is created to alleviate the need for two checks: aer_cap and get_firmware_first().

Signed-off-by Alexandru Gagniuc <[email protected]>

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

drivers/pci/pcie/aer.c +8 -5

index a2e88386af28..85c3e173c025 100644
@@ -307,6 +307,12 @@ int pcie_aer_get_firmware_first(struct pci_dev *dev)
  		aer_set_firmware_first(dev);
  	return dev->__aer_firmware_first;
  }
+
+static bool pcie_aer_is_kernel_first(struct pci_dev *dev)
+{
+	return !!dev->aer_cap && !pcie_aer_get_firmware_first(dev);
+}
+
  #define	PCI_EXP_AER_FLAGS	(PCI_EXP_DEVCTL_CERE | PCI_EXP_DEVCTL_NFERE | \
  				 PCI_EXP_DEVCTL_FERE | PCI_EXP_DEVCTL_URRE)
 

@@ -337,10 +343,7 @@ bool aer_acpi_firmware_first(void)
 
  int pci_enable_pcie_error_reporting(struct pci_dev *dev)
  {
-	if (pcie_aer_get_firmware_first(dev))
-		return -EIO;
-
-	if (!dev->aer_cap)
+	if (!pcie_aer_is_kernel_first(dev))
  		return -EIO;
 
  	return pcie_capability_set_word(dev, PCI_EXP_DEVCTL, PCI_EXP_AER_FLAGS);

@@ -349,7 +352,7 @@ EXPORT_SYMBOL_GPL(pci_enable_pcie_error_reporting);
 
  int pci_disable_pcie_error_reporting(struct pci_dev *dev)
  {
-	if (pcie_aer_get_firmware_first(dev))
+	if (!pcie_aer_is_kernel_first(dev))
  		return -EIO;
 
  	return pcie_capability_clear_word(dev, PCI_EXP_DEVCTL,

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

  		return -EIO;
 
+	pos = dev->aer_cap;
  	port_type = pci_pcie_type(dev);
  	if (port_type == PCI_EXP_TYPE_ROOT_PORT) {
  		pci_read_config_dword(dev, pos + PCI_ERR_ROOT_STATUS, &status);

When we don't own AER, we shouldn't touch the AER error bits. Clearing error bits willy-nilly might cause firmware to miss some errors. In theory, these bits get cleared by FFS, or via ACPI _HPX method. These mechanisms are not subject to the problem.

This race is mostly of theoretical significance, since I can't reasonably demonstrate this race in the lab.

On a side-note, pcie_aer_is_kernel_first() is created to alleviate the need for two checks: aer_cap and get_firmware_first().

Signed-off-by Alexandru Gagniuc <[email protected]>

---

Changes since v2:
- Added missing negation in pci_cleanup_aer_error_status_regs()

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

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

index a2e88386af28..40e5c86271d1 100644

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

 #define	PCI_EXP_AER_FLAGS	(PCI_EXP_DEVCTL_CERE | PCI_EXP_DEVCTL_NFERE | \
 				 PCI_EXP_DEVCTL_FERE | PCI_EXP_DEVCTL_URRE)
 

@@ -337,10 +343,7 @@ bool aer_acpi_firmware_first(void)
 

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

+	pos = dev->aer_cap;
 	port_type = pci_pcie_type(dev);
 	if (port_type == PCI_EXP_TYPE_ROOT_PORT) {
 		pci_read_config_dword(dev, pos + PCI_ERR_ROOT_STATUS, &status);

Signature · 1 line

2.17.1

Quoted 4 lines · Alexandru Gagniuc

When we don't own AER, we shouldn't touch the AER error bits. Clearing error bits willy-nilly might cause firmware to miss some errors. In theory, these bits get cleared by FFS, or via ACPI _HPX method. These mechanisms are not subject to the problem.

What's FFS?

I guess you mean FFS and _HPX are not subject to the problem because they're supplied by firmware, so firmware would be responsible for looking at the bits before clearing them?

Quoted 20 lines

This race is mostly of theoretical significance, since I can't reasonably demonstrate this race in the lab.

On a side-note, pcie_aer_is_kernel_first() is created to alleviate the need for two checks: aer_cap and get_firmware_first().

Signed-off-by Alexandru Gagniuc <[email protected]>

---

Changes since v2:
- Added missing negation in pci_cleanup_aer_error_status_regs()

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

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

index a2e88386af28..40e5c86271d1 100644

drivers/pci/pcie/aer.c int pcie_aer_get_firmware_first(struct pci_dev *dev) · line 314

Quoted 8 lines

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

@@ -307,6 +307,12 @@ int pcie_aer_get_firmware_first(struct pci_dev *dev)
  		aer_set_firmware_first(dev);
  	return dev->__aer_firmware_first;
  }
+
+static bool pcie_aer_is_kernel_first(struct pci_dev *dev)
+{
+	return !!dev->aer_cap && !pcie_aer_get_firmware_first(dev);
+}

I think it complicates things to have both "firmware_first" and "kernel_first" interfaces, so I would prefer to stick with the existing "firmware_first" style.

Quoted 14 lines

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

  #define	PCI_EXP_AER_FLAGS	(PCI_EXP_DEVCTL_CERE | PCI_EXP_DEVCTL_NFERE | \
  				 PCI_EXP_DEVCTL_FERE | PCI_EXP_DEVCTL_URRE)
 

@@ -337,10 +343,7 @@ bool aer_acpi_firmware_first(void)
 
  int pci_enable_pcie_error_reporting(struct pci_dev *dev)
  {
-	if (pcie_aer_get_firmware_first(dev))
-		return -EIO;
-
-	if (!dev->aer_cap)
+	if (!pcie_aer_is_kernel_first(dev))
  		return -EIO;
 

drivers/pci/pcie/aer.c bool aer_acpi_firmware_first(void) · line 349

Quoted 1 line

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

  	return pcie_capability_set_word(dev, PCI_EXP_DEVCTL, PCI_EXP_AER_FLAGS);

This change doesn't actually fix anything, does it? It looks like a cleanup that doesn't change the behavior.

drivers/pci/pcie/aer.c EXPORT_SYMBOL_GPL(pci_enable_pcie_error_reporting); · line 356

Quoted 7 lines

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

@@ -349,7 +352,7 @@ EXPORT_SYMBOL_GPL(pci_enable_pcie_error_reporting);
 
  int pci_disable_pcie_error_reporting(struct pci_dev *dev)
  {
-	if (pcie_aer_get_firmware_first(dev))
+	if (!pcie_aer_is_kernel_first(dev))
  		return -EIO;

This change does effectively add a test for dev->aer_cap. That makes sense in terms of symmetry with pci_enable_pcie_error_reporting(), but I think it should be a separate patch because it's conceptually separate from the change below.

We should keep the existing behavior (but add the symmetry) here for now, but it's not clear to me that these paths should care about AER or firmware-first at all. PCI_EXP_DEVCTL is not an AER register and we have the _HPX mechanism for firmware to influence it (which these paths currently ignore). I suspect we should program these reporting enable bits in the core enumeration path instead of having drivers call these interfaces.

If/when we make changes along these lines, the history will be easier to follow if *this* change is not connected with the change below to pci_cleanup_aer_error_status_regs().

Quoted 5 lines

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

  	return pcie_capability_clear_word(dev, PCI_EXP_DEVCTL,

@@ -383,10 +386,10 @@ int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
  	if (!pci_is_pcie(dev))
  		return -ENODEV;
 

drivers/pci/pcie/aer.c int pci_cleanup_aer_error_status_regs(struct pci_dev *dev) · line 390

Quoted 4 lines

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

-	pos = dev->aer_cap;
-	if (!pos)
+	if (!pcie_aer_is_kernel_first(dev))
  		return -EIO;

This part makes sense to me, but I think I would rather have it match the existing style in pci_enable_pcie_error_reporting(), i.e., keep the test for dev->aer_cap and add a test for pcie_aer_get_firmware_first().

Quoted 7 lines

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

+	pos = dev->aer_cap;
  	port_type = pci_pcie_type(dev);
  	if (port_type == PCI_EXP_TYPE_ROOT_PORT) {
  		pci_read_config_dword(dev, pos + PCI_ERR_ROOT_STATUS, &status);

Signature · 1 line

2.17.1

When we don't own AER, we shouldn't touch the AER error bits. This happens unconditionally on device probe(). Clearing AER bits willy-nilly might cause firmware to miss errors. Instead these bits should get cleared by FFS, or via ACPI _HPX method.

This race is mostly of theoretical significance, as it is not easy to reasonably demonstrate it in testing.

Signed-off-by Alexandru Gagniuc <[email protected]>

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

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

index a2e88386af28..18037a2a8231 100644

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

 	pos = dev->aer_cap;
 	if (!pos)
 		return -EIO;

Signature · 1 line

2.14.3

On Tue, Jul 17, 2018 at 10:31:23AM -0500, Alexandru Gagniuc wrote:=0A=

Quoted 10 lines

When we don't own AER, we shouldn't touch the AER error bits. This=0A= happens unconditionally on device probe(). Clearing AER bits=0A= willy-nilly might cause firmware to miss errors. Instead=0A= these bits should get cleared by FFS, or via ACPI _HPX method.=0A= =0A=
This race is mostly of theoretical significance, as it is not easy to=0A= reasonably demonstrate it in testing.=0A=
=0A=

Signed-off-by Alexandru Gagniuc <[email protected]>=0A=

---=0A=

drivers/pci/pcie/aer.c=0A= int pci_cleanup_aer_error_status_regs(struct pci_dev=

Quoted 8 lines

drivers/pci/pcie/aer.c | 3 +++=0A=

  1 file changed, 3 insertions(+)=0A=

=0A=

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

index a2e88386af28..18037a2a8231 100644=0A=
@@ -383,6 +383,9 @@ int pci_cleanup_aer_error_status_regs(struct pci_dev=

drivers/pci/pcie/aer.c=0A= int pci_cleanup_aer_error_status_regs(struct pci_dev= · line 387

Quoted 5 lines

drivers/pci/pcie/aer.c=0A= +2 -0

  	if (!pci_is_pcie(dev))=0A=
  		return -ENODEV;=0A=
  =0A=
+	if (pcie_aer_get_firmware_first(dev))=0A=
+		return -EIO;=0A=

=0A=
I like this patch.=0A=
=0A=
Do we need the same thing in the following places that also clear AER=0A= status bits or write AER control bits?=0A=

   enable_ecrc_checking()=0A=
   disable_ecrc_checking()=0A=

pci_cleanup_aer_uncorrect_error_status()=0A=

   pci_aer_clear_fatal_status()=0A=

Quoted 6 lines

drivers/pci/pcie/aer.c=0A= +0 -1

  	pos =3D dev->aer_cap;=0A=
  	if (!pos)=0A=
  		return -EIO;=0A=
-- =0A=

2.14.3=0A=
=0A=

=0A=

Quoted 22 lines · Bjorn Helgaas

Quoted 21 lines · Alexandru Gagniuc

When we don't own AER, we shouldn't touch the AER error bits. This happens unconditionally on device probe(). Clearing AER bits willy-nilly might cause firmware to miss errors. Instead these bits should get cleared by FFS, or via ACPI _HPX method.

This race is mostly of theoretical significance, as it is not easy to reasonably demonstrate it in testing.

Signed-off-by Alexandru Gagniuc <[email protected]>

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

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

index a2e88386af28..18037a2a8231 100644
@@ -383,6 +383,9 @@ int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
  	if (!pci_is_pcie(dev))
  		return -ENODEV;
 

Quoted 7 lines

drivers/pci/pcie/aer.c int pci_cleanup_aer_error_status_regs(struct pci_dev *dev) · line 387

Quoted 2 lines

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

+	if (pcie_aer_get_firmware_first(dev))
+		return -EIO;

I like this patch.

Do we need the same thing in the following places that also clear AER status bits or write AER control bits?

In theory, every exported function would guard for this. I think the idea a long long time ago was that the check happens during initialization, and the others are not hit.

Quoted 2 lines

enable_ecrc_checking()
disable_ecrc_checking()

I don't immediately see how this would affect FFS, but the bits are part of the AER capability structure. According to the FFS model, those would be owned by FW, and we'd have to avoid touching them.

Quoted 1 line

pci_cleanup_aer_uncorrect_error_status()

This probably should be guarded. It's only called from a few specific drivers, so the impact is not as high as being called from the core.

Quoted 1 line

pci_aer_clear_fatal_status()

This is only called when doing fatal_recovery, right?

For practical considerations this is not an issue today. The ACPI error handling code currently crashes when it encounters any fatal error, so we wouldn't hit this in the FFS case.

The PCIe standards contact I usually talk to about these PCIe subtleties is currently on vacation. The number one issue was a FFS corner case with OS clearing bits on probe. The other functions you mention are a corner case of a corner case. The big fish is pci_cleanup_aer_error_status_regs() on probe(), and it would be nice to have that resolved.

I'll sync up with Austin when he gets back to see about the other functions though I suspect we'll end up fixing them as well.

Quoted 1 line · On Thu, Aug 09, 2018 at 04:46:32PM +0000, [email protected] wrote:

On 08/09/2018 09:16 AM, Bjorn Helgaas wrote:

Quoted 6 lines

Quoted 2 lines

enable_ecrc_checking()
disable_ecrc_checking()

I don't immediately see how this would affect FFS, but the bits are part of the AER capability structure. According to the FFS model, those would be owned by FW, and we'd have to avoid touching them.

Per ACPI v6.2, sec 18.3.2.4, the HEST may contain entries for Root Ports that contain the FIRMWARE_FIRST flag as well as values the OS is supposed to write to several AER capability registers. It looks like we currently ignore everything except the FIRMWARE_FIRST and GLOBAL flags (ACPI_HEST_FIRMWARE_FIRST and ACPI_HEST_GLOBAL in Linux).

That seems like a pretty major screwup and more than I want to fix right now.

Quoted 5 lines

Quoted 1 line

pci_cleanup_aer_uncorrect_error_status()

This probably should be guarded. It's only called from a few specific drivers, so the impact is not as high as being called from the core.

Quoted 3 lines

Quoted 1 line

pci_aer_clear_fatal_status()

This is only called when doing fatal_recovery, right?

True. It takes a lot of analysis to convince oneself that this is not used in the firmware-first path, so I think we should add a guard there.

Quoted 3 lines

For practical considerations this is not an issue today. The ACPI error handling code currently crashes when it encounters any fatal error, so we wouldn't hit this in the FFS case.

I wasn't aware the firmware-first path was *that* broken. Are there problem reports for this? Is this a regression?

Quoted 7 lines

The PCIe standards contact I usually talk to about these PCIe subtleties is currently on vacation. The number one issue was a FFS corner case with OS clearing bits on probe. The other functions you mention are a corner case of a corner case. The big fish is pci_cleanup_aer_error_status_regs() on probe(), and it would be nice to have that resolved.

Quoted 2 lines

I'll sync up with Austin when he gets back to see about the other functions though I suspect we'll end up fixing them as well.

I'd like to fix all the obvious cases at once (excluding the ECRC stuff). What do you think about the following patch?

commit 15ed68dcc26864c849a12a36db4d4771bad7991f

Author: Alexandru Gagniuc <[email protected]>
Date:   Tue Jul 17 10:31:23 2018 -0500

PCI/AER: Don't clear AER bits if error handling is Firmware-First

If the platform requests Firmware-First error handling, firmware is responsible for reading and clearing AER status bits. If OSPM also clears them, we may miss errors. See ACPI v6.2, sec 18.3.2.5 and 18.4.

This race is mostly of theoretical significance, as it is not easy to reasonably demonstrate it in testing.

Signed-off-by Alexandru Gagniuc <[email protected]>

[bhelgaas: add similar guards to pci_cleanup_aer_uncorrect_error_status() and pci_aer_clear_fatal_status()]

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

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

index c6cc855bfa22..4e823ae051a7 100644
@@ -397,6 +397,9 @@ int pci_cleanup_aer_uncorrect_error_status(struct pci_dev *dev)
  	if (!pos)
  		return -EIO;
 
+	if (pcie_aer_get_firmware_first(dev))
+		return -EIO;
+
  	/* Clear status bits for ERR_NONFATAL errors only */
  	pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_STATUS, &status);
  	pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_SEVER, &sev);

@@ -417,6 +420,9 @@ void pci_aer_clear_fatal_status(struct pci_dev *dev)
  	if (!pos)
  		return;
 
+	if (pcie_aer_get_firmware_first(dev))
+		return;
+
  	/* Clear status bits for ERR_FATAL errors only */
  	pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_STATUS, &status);
  	pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_SEVER, &sev);

@@ -438,6 +444,9 @@ int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
  	if (!pos)
  		return -EIO;
 
+	if (pcie_aer_get_firmware_first(dev))
+		return -EIO;
+
  	port_type = pci_pcie_type(dev);
  	if (port_type == PCI_EXP_TYPE_ROOT_PORT) {
  		pci_read_config_dword(dev, pos + PCI_ERR_ROOT_STATUS, &status);

Quoted 2 lines · Bjorn Helgaas

Quoted 1 line · On Thu, Aug 09, 2018 at 04:46:32PM +0000, [email protected] wrote:

On 08/09/2018 09:16 AM, Bjorn Helgaas wrote:

(snip_

Quoted 13 lines

Quoted 6 lines

Quoted 2 lines

enable_ecrc_checking()
disable_ecrc_checking()

I don't immediately see how this would affect FFS, but the bits are part of the AER capability structure. According to the FFS model, those would be owned by FW, and we'd have to avoid touching them.

Per ACPI v6.2, sec 18.3.2.4, the HEST may contain entries for Root Ports that contain the FIRMWARE_FIRST flag as well as values the OS is supposed to write to several AER capability registers. It looks like we currently ignore everything except the FIRMWARE_FIRST and GLOBAL flags (ACPI_HEST_FIRMWARE_FIRST and ACPI_HEST_GLOBAL in Linux).

Quoted 2 lines

That seems like a pretty major screwup and more than I want to fix right now.

The logic is not very clear, but I think it goes like this:
For GLOBAL and FFS, disable native AER everywhere.
When !GLOBAL and FFS, then only disable native AER for the root port described by the HEST entry.

Quoted 4 lines

Quoted 3 lines

For practical considerations this is not an issue today. The ACPI error handling code currently crashes when it encounters any fatal error, so we wouldn't hit this in the FFS case.

Quoted 2 lines

I wasn't aware the firmware-first path was *that* broken. Are there problem reports for this? Is this a regression?

It's been like this since, I believe, 3.10, and probably much earlier. All reports that I have seen of linux crashing on surprise hot-plug have been caused by the panic() call in the apei code. Dell BIOSes do an extreme amount of work to determine when it's safe to _not_ report errors to the OS, since all known OSes crash on this path.

Quoted 17 lines · Alex G.

Quoted 2 lines · Bjorn Helgaas

Quoted 1 line · On Thu, Aug 09, 2018 at 04:46:32PM +0000, [email protected] wrote:

On 08/09/2018 09:16 AM, Bjorn Helgaas wrote:

(snip_

Quoted 13 lines

Quoted 6 lines

Quoted 2 lines

enable_ecrc_checking()
disable_ecrc_checking()

I don't immediately see how this would affect FFS, but the bits are part of the AER capability structure. According to the FFS model, those would be owned by FW, and we'd have to avoid touching them.

Per ACPI v6.2, sec 18.3.2.4, the HEST may contain entries for Root Ports that contain the FIRMWARE_FIRST flag as well as values the OS is supposed to write to several AER capability registers. It looks like we currently ignore everything except the FIRMWARE_FIRST and GLOBAL flags (ACPI_HEST_FIRMWARE_FIRST and ACPI_HEST_GLOBAL in Linux).

Quoted 7 lines

Quoted 2 lines

That seems like a pretty major screwup and more than I want to fix right now.

The logic is not very clear, but I think it goes like this:
For GLOBAL and FFS, disable native AER everywhere.
When !GLOBAL and FFS, then only disable native AER for the root port described by the HEST entry.

I agree the code is convoluted, but that sounds right to me.

What I meant is that we ignore the values the HEST entry tells us we're supposed to write to Device Control and the AER Uncorrectable Error Mask, Uncorrectable Error Severity, Correctable Error Mask, and AER Capabilities and Control.

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

For practical considerations this is not an issue today. The ACPI error handling code currently crashes when it encounters any fatal error, so we wouldn't hit this in the FFS case.

Quoted 8 lines

Quoted 2 lines

I wasn't aware the firmware-first path was *that* broken. Are there problem reports for this? Is this a regression?

It's been like this since, I believe, 3.10, and probably much earlier. All reports that I have seen of linux crashing on surprise hot-plug have been caused by the panic() call in the apei code. Dell BIOSes do an extreme amount of work to determine when it's safe to _not_ report errors to the OS, since all known OSes crash on this path.

Oh, is this the __ghes_panic() path? If so, I'm going to turn away and plead ignorance unless the PCI core is doing something wrong that eventually results in that panic.