linux-edac.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/3] PCI/AER: Handle Advisory Non-Fatal error
@ 2024-04-17  6:14 Zhenzhong Duan
  2024-04-17  6:14 ` [PATCH v3 1/3] PCI/AER: Store UNCOR_STATUS bits that might be ANFE in aer_err_info Zhenzhong Duan
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Zhenzhong Duan @ 2024-04-17  6:14 UTC (permalink / raw)
  To: linux-pci, linuxppc-dev, linux-acpi
  Cc: rafael, lenb, james.morse, tony.luck, bp, dave, jonathan.cameron,
	dave.jiang, alison.schofield, vishal.l.verma, ira.weiny,
	bhelgaas, helgaas, mahesh, oohall, linmiaohe, shiju.jose,
	adam.c.preble, leoyang.li, lukas, Smita.KoralahalliChannabasappa,
	rrichter, linux-cxl, linux-edac, linux-kernel, erwin.tsaur,
	sathyanarayanan.kuppuswamy, dan.j.williams, feiting.wanyan,
	yudong.wang, chao.p.peng, qingshun.wang, Zhenzhong Duan

Hi,

This is a relay work of Qingshun's v2 [1], but changed to focus on ANFE
processing as subject suggests and drops trace-event for now. I think it's
a bit heavy to do extra IOes to get PCIe registers only for trace purpose
and not see it a community request for now.

According to PCIe Base Specification Revision 6.1, Sections 6.2.3.2.4 and
6.2.4.3, certain uncorrectable errors will signal ERR_COR instead of
ERR_NONFATAL, logged as Advisory Non-Fatal Error(ANFE), and set bits in
both Correctable Error(CE) Status register and Uncorrectable Error(UE)
Status register. Currently, when handling AER events the kernel will only
look at CE status or UE status, but never both. In the ANFE case, bits set
in the UE status register will not be reported and cleared until the next
FE/NFE arrives.

For instance, previously, when the kernel receives an ANFE with Poisoned
TLP in OS native AER mode, only the status of CE will be reported and
cleared:

  AER: Correctable error message received from 0000:b7:02.0
  PCIe Bus Error: severity=Correctable, type=Transaction Layer, (Receiver ID)
    device [8086:0db0] error status/mask=00002000/00000000
     [13] NonFatalErr

If the kernel receives a Malformed TLP after that, two UEs will be
reported, which is unexpected. The Malformed TLP Header is lost since
the previous ANFE gated the TLP header logs:

  PCIe Bus Error: severity="Uncorrectable (Fatal), type=Transaction Layer, (Receiver ID)
    device [8086:0db0] error status/mask=00041000/00180020
     [12] TLP                    (First)
     [18] MalfTLP

To handle this case properly, calculate potential ANFE related status bits
and save in aer_err_info. Use this information to determine the status bits
that need to be cleared.

Now, for the previous scenario, both CE status and related UE status will
be reported and cleared after ANFE:

  AER: Correctable error message received from 0000:b7:02.0
  PCIe Bus Error: severity=Correctable, type=Transaction Layer, (Receiver ID)
    device [8086:0db0] error status/mask=00002000/00000000
     [13] NonFatalErr
    Uncorrectable errors that may cause Advisory Non-Fatal:
     [18] TLP

Note:
checkpatch.pl will produce following warnings on PATCH2/3:

WARNING: 'UE' may be misspelled - perhaps 'USE'?
#22:
uncorrectable error(UE) status should be cleared. However, there is no

...similar warnings omitted...

This is a false-positive, so not fixed.

WARNING: Prefer a maximum 75 chars per line (possible unwrapped commit description?)
#10:
  PCIe Bus Error: severity=Correctable, type=Transaction Layer, (Receiver ID)

...similar warnings omitted...

For readability reasons, these warnings are not fixed.



[1] https://lore.kernel.org/linux-pci/20240125062802.50819-1-qingshun.wang@linux.intel.com

Thanks
Qingshun, Zhenzhong

Changelog:
v3:
  - Split ANFE print and processing to two patches (Bjorn)
  - Simplify ANFE handling, drop trace event
  - Polish comments and patch description
  - Add Tested-by

v2:
  - Reference to the latest PCIe Specification in both commit messages
    and comments, as suggested by Bjorn Helgaas.
  - Describe the reason for storing additional information in
    aer_err_info in the commit message of PATCH 1, as suggested by Bjorn
    Helgaas.
  - Add more details of behavior changes in the commit message of PATCH
    2, as suggested by Bjorn Helgaas.

v1: https://lore.kernel.org/linux-pci/20240111073227.31488-1-qingshun.wang@linux.intel.com/


Zhenzhong Duan (3):
  PCI/AER: Store UNCOR_STATUS bits that might be ANFE in aer_err_info
  PCI/AER: Print UNCOR_STATUS bits that might be ANFE
  PCI/AER: Clear UNCOR_STATUS bits that might be ANFE

 drivers/pci/pci.h      |  1 +
 drivers/pci/pcie/aer.c | 67 +++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 67 insertions(+), 1 deletion(-)

-- 
2.34.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v3 1/3] PCI/AER: Store UNCOR_STATUS bits that might be ANFE in aer_err_info
  2024-04-17  6:14 [PATCH v3 0/3] PCI/AER: Handle Advisory Non-Fatal error Zhenzhong Duan
@ 2024-04-17  6:14 ` Zhenzhong Duan
  2024-04-22 16:16   ` Jonathan Cameron
  2024-04-17  6:14 ` [PATCH v3 2/3] PCI/AER: Print UNCOR_STATUS bits that might be ANFE Zhenzhong Duan
  2024-04-17  6:14 ` [PATCH v3 3/3] PCI/AER: Clear " Zhenzhong Duan
  2 siblings, 1 reply; 8+ messages in thread
From: Zhenzhong Duan @ 2024-04-17  6:14 UTC (permalink / raw)
  To: linux-pci, linuxppc-dev, linux-acpi
  Cc: rafael, lenb, james.morse, tony.luck, bp, dave, jonathan.cameron,
	dave.jiang, alison.schofield, vishal.l.verma, ira.weiny,
	bhelgaas, helgaas, mahesh, oohall, linmiaohe, shiju.jose,
	adam.c.preble, leoyang.li, lukas, Smita.KoralahalliChannabasappa,
	rrichter, linux-cxl, linux-edac, linux-kernel, erwin.tsaur,
	sathyanarayanan.kuppuswamy, dan.j.williams, feiting.wanyan,
	yudong.wang, chao.p.peng, qingshun.wang, Zhenzhong Duan

In some cases the detector of a Non-Fatal Error(NFE) is not the most
appropriate agent to determine the type of the error. For example,
when software performs a configuration read from a non-existent
device or Function, completer will send an ERR_NONFATAL Message.
On some platforms, ERR_NONFATAL results in a System Error, which
breaks normal software probing.

Advisory Non-Fatal Error(ANFE) is a special case that can be used
in above scenario. It is predominantly determined by the role of the
detecting agent (Requester, Completer, or Receiver) and the specific
error. In such cases, an agent with AER signals the NFE (if enabled)
by sending an ERR_COR Message as an advisory to software, instead of
sending ERR_NONFATAL.

When processing an ANFE, ideally both correctable error(CE) status and
uncorrectable error(UE) status should be cleared. However, there is no
way to fully identify the UE associated with ANFE. Even worse, a Fatal
Error(FE) or Non-Fatal Error(NFE) may set the same UE status bit as
ANFE. Treating an ANFE as NFE will reproduce above mentioned issue,
i.e., breaking softwore probing; treating NFE as ANFE will make us
ignoring some UEs which need active recover operation. To avoid clearing
UEs that are not ANFE by accident, the most conservative route is taken
here: If any of the FE/NFE Detected bits is set in Device Status, do not
touch UE status, they should be cleared later by the UE handler. Otherwise,
a specific set of UEs that may be raised as ANFE according to the PCIe
specification will be cleared if their corresponding severity is Non-Fatal.

To achieve above purpose, store UNCOR_STATUS bits that might be ANFE
in aer_err_info.anfe_status. So that those bits could be printed and
processed later.

Tested-by: Yudong Wang <yudong.wang@intel.com>
Co-developed-by: "Wang, Qingshun" <qingshun.wang@linux.intel.com>
Signed-off-by: "Wang, Qingshun" <qingshun.wang@linux.intel.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 drivers/pci/pci.h      |  1 +
 drivers/pci/pcie/aer.c | 45 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+)

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 17fed1846847..3f9eb807f9fd 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -412,6 +412,7 @@ struct aer_err_info {
 
 	unsigned int status;		/* COR/UNCOR Error Status */
 	unsigned int mask;		/* COR/UNCOR Error Mask */
+	unsigned int anfe_status;	/* UNCOR Error Status for ANFE */
 	struct pcie_tlp_log tlp;	/* TLP Header */
 };
 
diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index ac6293c24976..27364ab4b148 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -107,6 +107,12 @@ struct aer_stats {
 					PCI_ERR_ROOT_MULTI_COR_RCV |	\
 					PCI_ERR_ROOT_MULTI_UNCOR_RCV)
 
+#define AER_ERR_ANFE_UNC_MASK		(PCI_ERR_UNC_POISON_TLP |	\
+					PCI_ERR_UNC_COMP_TIME |		\
+					PCI_ERR_UNC_COMP_ABORT |	\
+					PCI_ERR_UNC_UNX_COMP |		\
+					PCI_ERR_UNC_UNSUP)
+
 static int pcie_aer_disable;
 static pci_ers_result_t aer_root_reset(struct pci_dev *dev);
 
@@ -1196,6 +1202,41 @@ void aer_recover_queue(int domain, unsigned int bus, unsigned int devfn,
 EXPORT_SYMBOL_GPL(aer_recover_queue);
 #endif
 
+static void anfe_get_uc_status(struct pci_dev *dev, struct aer_err_info *info)
+{
+	u32 uncor_mask, uncor_status;
+	u16 device_status;
+	int aer = dev->aer_cap;
+
+	if (pcie_capability_read_word(dev, PCI_EXP_DEVSTA, &device_status))
+		return;
+	/*
+	 * Take the most conservative route here. If there are
+	 * Non-Fatal/Fatal errors detected, do not assume any
+	 * bit in uncor_status is set by ANFE.
+	 */
+	if (device_status & (PCI_EXP_DEVSTA_NFED | PCI_EXP_DEVSTA_FED))
+		return;
+
+	pci_read_config_dword(dev, aer + PCI_ERR_UNCOR_STATUS, &uncor_status);
+	pci_read_config_dword(dev, aer + PCI_ERR_UNCOR_MASK, &uncor_mask);
+	/*
+	 * According to PCIe Base Specification Revision 6.1,
+	 * Section 6.2.3.2.4, if an UNCOR error is raised as
+	 * Advisory Non-Fatal error, it will match the following
+	 * conditions:
+	 *	a. The severity of the error is Non-Fatal.
+	 *	b. The error is one of the following:
+	 *		1. Poisoned TLP           (Section 6.2.3.2.4.3)
+	 *		2. Completion Timeout     (Section 6.2.3.2.4.4)
+	 *		3. Completer Abort        (Section 6.2.3.2.4.1)
+	 *		4. Unexpected Completion  (Section 6.2.3.2.4.5)
+	 *		5. Unsupported Request    (Section 6.2.3.2.4.1)
+	 */
+	info->anfe_status = uncor_status & ~uncor_mask & ~info->severity &
+			    AER_ERR_ANFE_UNC_MASK;
+}
+
 /**
  * aer_get_device_error_info - read error status from dev and store it to info
  * @dev: pointer to the device expected to have a error record
@@ -1213,6 +1254,7 @@ int aer_get_device_error_info(struct pci_dev *dev, struct aer_err_info *info)
 
 	/* Must reset in this function */
 	info->status = 0;
+	info->anfe_status = 0;
 	info->tlp_header_valid = 0;
 
 	/* The device might not support AER */
@@ -1226,6 +1268,9 @@ int aer_get_device_error_info(struct pci_dev *dev, struct aer_err_info *info)
 			&info->mask);
 		if (!(info->status & ~info->mask))
 			return 0;
+
+		if (info->status & PCI_ERR_COR_ADV_NFAT)
+			anfe_get_uc_status(dev, info);
 	} else if (type == PCI_EXP_TYPE_ROOT_PORT ||
 		   type == PCI_EXP_TYPE_RC_EC ||
 		   type == PCI_EXP_TYPE_DOWNSTREAM ||
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v3 2/3] PCI/AER: Print UNCOR_STATUS bits that might be ANFE
  2024-04-17  6:14 [PATCH v3 0/3] PCI/AER: Handle Advisory Non-Fatal error Zhenzhong Duan
  2024-04-17  6:14 ` [PATCH v3 1/3] PCI/AER: Store UNCOR_STATUS bits that might be ANFE in aer_err_info Zhenzhong Duan
@ 2024-04-17  6:14 ` Zhenzhong Duan
  2024-04-17  6:14 ` [PATCH v3 3/3] PCI/AER: Clear " Zhenzhong Duan
  2 siblings, 0 replies; 8+ messages in thread
From: Zhenzhong Duan @ 2024-04-17  6:14 UTC (permalink / raw)
  To: linux-pci, linuxppc-dev, linux-acpi
  Cc: rafael, lenb, james.morse, tony.luck, bp, dave, jonathan.cameron,
	dave.jiang, alison.schofield, vishal.l.verma, ira.weiny,
	bhelgaas, helgaas, mahesh, oohall, linmiaohe, shiju.jose,
	adam.c.preble, leoyang.li, lukas, Smita.KoralahalliChannabasappa,
	rrichter, linux-cxl, linux-edac, linux-kernel, erwin.tsaur,
	sathyanarayanan.kuppuswamy, dan.j.williams, feiting.wanyan,
	yudong.wang, chao.p.peng, qingshun.wang, Zhenzhong Duan

When an Advisory Non-Fatal error(ANFE) triggers, both correctable error(CE)
status and ANFE related uncorrectable error(UE) status will be printed:

  AER: Correctable error message received from 0000:b7:02.0
  PCIe Bus Error: severity=Correctable, type=Transaction Layer, (Receiver ID)
    device [8086:0db0] error status/mask=00002000/00000000
     [13] NonFatalErr
    Uncorrectable errors that may cause Advisory Non-Fatal:
     [18] TLP

Tested-by: Yudong Wang <yudong.wang@intel.com>
Co-developed-by: "Wang, Qingshun" <qingshun.wang@linux.intel.com>
Signed-off-by: "Wang, Qingshun" <qingshun.wang@linux.intel.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 drivers/pci/pcie/aer.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index 27364ab4b148..870e1d1a5159 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -681,6 +681,7 @@ static void __aer_print_error(struct pci_dev *dev,
 {
 	const char **strings;
 	unsigned long status = info->status & ~info->mask;
+	unsigned long anfe_status = info->anfe_status;
 	const char *level, *errmsg;
 	int i;
 
@@ -701,6 +702,20 @@ static void __aer_print_error(struct pci_dev *dev,
 				info->first_error == i ? " (First)" : "");
 	}
 	pci_dev_aer_stats_incr(dev, info);
+
+	if (!anfe_status)
+		return;
+
+	strings = aer_uncorrectable_error_string;
+	pci_printk(level, dev, "Uncorrectable errors that may cause Advisory Non-Fatal:\n");
+
+	for_each_set_bit(i, &anfe_status, 32) {
+		errmsg = strings[i];
+		if (!errmsg)
+			errmsg = "Unknown Error Bit";
+
+		pci_printk(level, dev, "   [%2d] %s\n", i, errmsg);
+	}
 }
 
 void aer_print_error(struct pci_dev *dev, struct aer_err_info *info)
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v3 3/3] PCI/AER: Clear UNCOR_STATUS bits that might be ANFE
  2024-04-17  6:14 [PATCH v3 0/3] PCI/AER: Handle Advisory Non-Fatal error Zhenzhong Duan
  2024-04-17  6:14 ` [PATCH v3 1/3] PCI/AER: Store UNCOR_STATUS bits that might be ANFE in aer_err_info Zhenzhong Duan
  2024-04-17  6:14 ` [PATCH v3 2/3] PCI/AER: Print UNCOR_STATUS bits that might be ANFE Zhenzhong Duan
@ 2024-04-17  6:14 ` Zhenzhong Duan
  2 siblings, 0 replies; 8+ messages in thread
From: Zhenzhong Duan @ 2024-04-17  6:14 UTC (permalink / raw)
  To: linux-pci, linuxppc-dev, linux-acpi
  Cc: rafael, lenb, james.morse, tony.luck, bp, dave, jonathan.cameron,
	dave.jiang, alison.schofield, vishal.l.verma, ira.weiny,
	bhelgaas, helgaas, mahesh, oohall, linmiaohe, shiju.jose,
	adam.c.preble, leoyang.li, lukas, Smita.KoralahalliChannabasappa,
	rrichter, linux-cxl, linux-edac, linux-kernel, erwin.tsaur,
	sathyanarayanan.kuppuswamy, dan.j.williams, feiting.wanyan,
	yudong.wang, chao.p.peng, qingshun.wang, Zhenzhong Duan

When processing an ANFE, ideally both correctable error(CE) status and
uncorrectable error(UE) status should be cleared. However, there is no
way to fully identify the UE associated with ANFE. Even worse, a Fatal
Error(FE) or Non-Fatal Error(NFE) may set the same UE status bit as
ANFE. Treating an ANFE as NFE will reproduce above mentioned issue,
i.e., breaking softwore probing; treating NFE as ANFE will make us
ignoring some UEs which need active recover operation. To avoid clearing
UEs that are not ANFE by accident, the most conservative route is taken
here: If any of the FE/NFE Detected bits is set in Device Status, do not
touch UE status, they should be cleared later by the UE handler. Otherwise,
a specific set of UEs that may be raised as ANFE according to the PCIe
specification will be cleared if their corresponding severity is Non-Fatal.

For instance, previously when kernel receives an ANFE with Poisoned TLP
in OS native AER mode, only status of CE will be reported and cleared:

  AER: Correctable error message received from 0000:b7:02.0
  PCIe Bus Error: severity=Correctable, type=Transaction Layer, (Receiver ID)
    device [8086:0db0] error status/mask=00002000/00000000
     [13] NonFatalErr

If the kernel receives a Malformed TLP after that, two UEs will be
reported, which is unexpected. Malformed TLP Header is lost since
the previous ANFE gated the TLP header logs:

  PCIe Bus Error: severity="Uncorrectable (Fatal), type=Transaction Layer, (Receiver ID)
    device [8086:0db0] error status/mask=00041000/00180020
     [12] TLP                    (First)
     [18] MalfTLP

Now, for the same scenario, both CE status and related UE status will be
reported and cleared after ANFE:

  AER: Correctable error message received from 0000:b7:02.0
  PCIe Bus Error: severity=Correctable, type=Transaction Layer, (Receiver ID)
    device [8086:0db0] error status/mask=00002000/00000000
     [13] NonFatalErr
    Uncorrectable errors that may cause Advisory Non-Fatal:
     [18] TLP

Tested-by: Yudong Wang <yudong.wang@intel.com>
Co-developed-by: "Wang, Qingshun" <qingshun.wang@linux.intel.com>
Signed-off-by: "Wang, Qingshun" <qingshun.wang@linux.intel.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 drivers/pci/pcie/aer.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index 870e1d1a5159..6ebe320eb0f7 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -1115,9 +1115,14 @@ static void pci_aer_handle_error(struct pci_dev *dev, struct aer_err_info *info)
 		 * Correctable error does not need software intervention.
 		 * No need to go through error recovery process.
 		 */
-		if (aer)
+		if (aer) {
 			pci_write_config_dword(dev, aer + PCI_ERR_COR_STATUS,
 					info->status);
+			if (info->anfe_status)
+				pci_write_config_dword(dev,
+						       aer + PCI_ERR_UNCOR_STATUS,
+						       info->anfe_status);
+		}
 		if (pcie_aer_is_native(dev)) {
 			struct pci_driver *pdrv = dev->driver;
 
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v3 1/3] PCI/AER: Store UNCOR_STATUS bits that might be ANFE in aer_err_info
  2024-04-17  6:14 ` [PATCH v3 1/3] PCI/AER: Store UNCOR_STATUS bits that might be ANFE in aer_err_info Zhenzhong Duan
@ 2024-04-22 16:16   ` Jonathan Cameron
  2024-04-23  2:25     ` Duan, Zhenzhong
  0 siblings, 1 reply; 8+ messages in thread
From: Jonathan Cameron @ 2024-04-22 16:16 UTC (permalink / raw)
  To: Zhenzhong Duan
  Cc: linux-pci, linuxppc-dev, linux-acpi, rafael, lenb, james.morse,
	tony.luck, bp, dave, dave.jiang, alison.schofield,
	vishal.l.verma, ira.weiny, bhelgaas, helgaas, mahesh, oohall,
	linmiaohe, shiju.jose, adam.c.preble, leoyang.li, lukas,
	Smita.KoralahalliChannabasappa, rrichter, linux-cxl, linux-edac,
	linux-kernel, erwin.tsaur, sathyanarayanan.kuppuswamy,
	dan.j.williams, feiting.wanyan, yudong.wang, chao.p.peng,
	qingshun.wang

On Wed, 17 Apr 2024 14:14:05 +0800
Zhenzhong Duan <zhenzhong.duan@intel.com> wrote:

> In some cases the detector of a Non-Fatal Error(NFE) is not the most
> appropriate agent to determine the type of the error. For example,
> when software performs a configuration read from a non-existent
> device or Function, completer will send an ERR_NONFATAL Message.
> On some platforms, ERR_NONFATAL results in a System Error, which
> breaks normal software probing.
> 
> Advisory Non-Fatal Error(ANFE) is a special case that can be used
> in above scenario. It is predominantly determined by the role of the
> detecting agent (Requester, Completer, or Receiver) and the specific
> error. In such cases, an agent with AER signals the NFE (if enabled)
> by sending an ERR_COR Message as an advisory to software, instead of
> sending ERR_NONFATAL.
> 
> When processing an ANFE, ideally both correctable error(CE) status and
> uncorrectable error(UE) status should be cleared. However, there is no
> way to fully identify the UE associated with ANFE. Even worse, a Fatal
> Error(FE) or Non-Fatal Error(NFE) may set the same UE status bit as
> ANFE. Treating an ANFE as NFE will reproduce above mentioned issue,
> i.e., breaking softwore probing; treating NFE as ANFE will make us
> ignoring some UEs which need active recover operation. To avoid clearing
> UEs that are not ANFE by accident, the most conservative route is taken
> here: If any of the FE/NFE Detected bits is set in Device Status, do not
> touch UE status, they should be cleared later by the UE handler. Otherwise,
> a specific set of UEs that may be raised as ANFE according to the PCIe
> specification will be cleared if their corresponding severity is Non-Fatal.
> 
> To achieve above purpose, store UNCOR_STATUS bits that might be ANFE
> in aer_err_info.anfe_status. So that those bits could be printed and
> processed later.
> 
> Tested-by: Yudong Wang <yudong.wang@intel.com>
> Co-developed-by: "Wang, Qingshun" <qingshun.wang@linux.intel.com>
> Signed-off-by: "Wang, Qingshun" <qingshun.wang@linux.intel.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>  drivers/pci/pci.h      |  1 +
>  drivers/pci/pcie/aer.c | 45 ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 46 insertions(+)
> 
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 17fed1846847..3f9eb807f9fd 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -412,6 +412,7 @@ struct aer_err_info {
>  
>  	unsigned int status;		/* COR/UNCOR Error Status */
>  	unsigned int mask;		/* COR/UNCOR Error Mask */
> +	unsigned int anfe_status;	/* UNCOR Error Status for ANFE */
>  	struct pcie_tlp_log tlp;	/* TLP Header */
>  };
>  
> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> index ac6293c24976..27364ab4b148 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -107,6 +107,12 @@ struct aer_stats {
>  					PCI_ERR_ROOT_MULTI_COR_RCV |	\
>  					PCI_ERR_ROOT_MULTI_UNCOR_RCV)
>  
> +#define AER_ERR_ANFE_UNC_MASK		(PCI_ERR_UNC_POISON_TLP |	\
> +					PCI_ERR_UNC_COMP_TIME |		\
> +					PCI_ERR_UNC_COMP_ABORT |	\
> +					PCI_ERR_UNC_UNX_COMP |		\
> +					PCI_ERR_UNC_UNSUP)
> +
>  static int pcie_aer_disable;
>  static pci_ers_result_t aer_root_reset(struct pci_dev *dev);
>  
> @@ -1196,6 +1202,41 @@ void aer_recover_queue(int domain, unsigned int bus, unsigned int devfn,
>  EXPORT_SYMBOL_GPL(aer_recover_queue);
>  #endif
>  
> +static void anfe_get_uc_status(struct pci_dev *dev, struct aer_err_info *info)
> +{
> +	u32 uncor_mask, uncor_status;
> +	u16 device_status;
> +	int aer = dev->aer_cap;
> +
> +	if (pcie_capability_read_word(dev, PCI_EXP_DEVSTA, &device_status))
> +		return;
> +	/*
> +	 * Take the most conservative route here. If there are
> +	 * Non-Fatal/Fatal errors detected, do not assume any
> +	 * bit in uncor_status is set by ANFE.
> +	 */
> +	if (device_status & (PCI_EXP_DEVSTA_NFED | PCI_EXP_DEVSTA_FED))
> +		return;
> +

Is there not a race here?  If we happen to get either an NFED or FED 
between the read of device_status above and here we might pick up a status
that corresponds to that (and hence clear something we should not).

Or am I missing that race being close somewhere?

> +	pci_read_config_dword(dev, aer + PCI_ERR_UNCOR_STATUS, &uncor_status);
> +	pci_read_config_dword(dev, aer + PCI_ERR_UNCOR_MASK, &uncor_mask);
> +	/*
> +	 * According to PCIe Base Specification Revision 6.1,
> +	 * Section 6.2.3.2.4, if an UNCOR error is raised as
> +	 * Advisory Non-Fatal error, it will match the following
> +	 * conditions:
> +	 *	a. The severity of the error is Non-Fatal.
> +	 *	b. The error is one of the following:
> +	 *		1. Poisoned TLP           (Section 6.2.3.2.4.3)
> +	 *		2. Completion Timeout     (Section 6.2.3.2.4.4)
> +	 *		3. Completer Abort        (Section 6.2.3.2.4.1)
> +	 *		4. Unexpected Completion  (Section 6.2.3.2.4.5)
> +	 *		5. Unsupported Request    (Section 6.2.3.2.4.1)
> +	 */
> +	info->anfe_status = uncor_status & ~uncor_mask & ~info->severity &
> +			    AER_ERR_ANFE_UNC_MASK;
> +}
> +
>  /**
>   * aer_get_device_error_info - read error status from dev and store it to info
>   * @dev: pointer to the device expected to have a error record
> @@ -1213,6 +1254,7 @@ int aer_get_device_error_info(struct pci_dev *dev, struct aer_err_info *info)
>  
>  	/* Must reset in this function */
>  	info->status = 0;
> +	info->anfe_status = 0;
>  	info->tlp_header_valid = 0;
>  
>  	/* The device might not support AER */
> @@ -1226,6 +1268,9 @@ int aer_get_device_error_info(struct pci_dev *dev, struct aer_err_info *info)
>  			&info->mask);
>  		if (!(info->status & ~info->mask))
>  			return 0;
> +
> +		if (info->status & PCI_ERR_COR_ADV_NFAT)
> +			anfe_get_uc_status(dev, info);
>  	} else if (type == PCI_EXP_TYPE_ROOT_PORT ||
>  		   type == PCI_EXP_TYPE_RC_EC ||
>  		   type == PCI_EXP_TYPE_DOWNSTREAM ||


^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH v3 1/3] PCI/AER: Store UNCOR_STATUS bits that might be ANFE in aer_err_info
  2024-04-22 16:16   ` Jonathan Cameron
@ 2024-04-23  2:25     ` Duan, Zhenzhong
  2024-04-26 16:11       ` Jonathan Cameron
  0 siblings, 1 reply; 8+ messages in thread
From: Duan, Zhenzhong @ 2024-04-23  2:25 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: linux-pci, linuxppc-dev, linux-acpi, rafael, lenb, james.morse,
	Luck, Tony, bp, dave, Jiang, Dave, Schofield, Alison, Verma,
	Vishal L, Weiny, Ira, bhelgaas, helgaas, mahesh, oohall,
	linmiaohe, shiju.jose, Preble, Adam C, leoyang.li, lukas,
	Smita.KoralahalliChannabasappa, rrichter, linux-cxl, linux-edac,
	linux-kernel, Tsaur, Erwin, Kuppuswamy, Sathyanarayanan,
	Williams, Dan J, Wanyan, Feiting, Wang, Yudong, Peng, Chao P,
	qingshun.wang



>-----Original Message-----
>From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
>Subject: Re: [PATCH v3 1/3] PCI/AER: Store UNCOR_STATUS bits that might
>be ANFE in aer_err_info
>
>On Wed, 17 Apr 2024 14:14:05 +0800
>Zhenzhong Duan <zhenzhong.duan@intel.com> wrote:
>
>> In some cases the detector of a Non-Fatal Error(NFE) is not the most
>> appropriate agent to determine the type of the error. For example,
>> when software performs a configuration read from a non-existent
>> device or Function, completer will send an ERR_NONFATAL Message.
>> On some platforms, ERR_NONFATAL results in a System Error, which
>> breaks normal software probing.
>>
>> Advisory Non-Fatal Error(ANFE) is a special case that can be used
>> in above scenario. It is predominantly determined by the role of the
>> detecting agent (Requester, Completer, or Receiver) and the specific
>> error. In such cases, an agent with AER signals the NFE (if enabled)
>> by sending an ERR_COR Message as an advisory to software, instead of
>> sending ERR_NONFATAL.
>>
>> When processing an ANFE, ideally both correctable error(CE) status and
>> uncorrectable error(UE) status should be cleared. However, there is no
>> way to fully identify the UE associated with ANFE. Even worse, a Fatal
>> Error(FE) or Non-Fatal Error(NFE) may set the same UE status bit as
>> ANFE. Treating an ANFE as NFE will reproduce above mentioned issue,
>> i.e., breaking softwore probing; treating NFE as ANFE will make us
>> ignoring some UEs which need active recover operation. To avoid clearing
>> UEs that are not ANFE by accident, the most conservative route is taken
>> here: If any of the FE/NFE Detected bits is set in Device Status, do not
>> touch UE status, they should be cleared later by the UE handler. Otherwise,
>> a specific set of UEs that may be raised as ANFE according to the PCIe
>> specification will be cleared if their corresponding severity is Non-Fatal.
>>
>> To achieve above purpose, store UNCOR_STATUS bits that might be ANFE
>> in aer_err_info.anfe_status. So that those bits could be printed and
>> processed later.
>>
>> Tested-by: Yudong Wang <yudong.wang@intel.com>
>> Co-developed-by: "Wang, Qingshun" <qingshun.wang@linux.intel.com>
>> Signed-off-by: "Wang, Qingshun" <qingshun.wang@linux.intel.com>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>  drivers/pci/pci.h      |  1 +
>>  drivers/pci/pcie/aer.c | 45
>++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 46 insertions(+)
>>
>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>> index 17fed1846847..3f9eb807f9fd 100644
>> --- a/drivers/pci/pci.h
>> +++ b/drivers/pci/pci.h
>> @@ -412,6 +412,7 @@ struct aer_err_info {
>>
>>  	unsigned int status;		/* COR/UNCOR Error Status */
>>  	unsigned int mask;		/* COR/UNCOR Error Mask */
>> +	unsigned int anfe_status;	/* UNCOR Error Status for ANFE */
>>  	struct pcie_tlp_log tlp;	/* TLP Header */
>>  };
>>
>> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
>> index ac6293c24976..27364ab4b148 100644
>> --- a/drivers/pci/pcie/aer.c
>> +++ b/drivers/pci/pcie/aer.c
>> @@ -107,6 +107,12 @@ struct aer_stats {
>>  					PCI_ERR_ROOT_MULTI_COR_RCV |
>	\
>>  					PCI_ERR_ROOT_MULTI_UNCOR_RCV)
>>
>> +#define AER_ERR_ANFE_UNC_MASK
>	(PCI_ERR_UNC_POISON_TLP |	\
>> +					PCI_ERR_UNC_COMP_TIME |
>	\
>> +					PCI_ERR_UNC_COMP_ABORT |
>	\
>> +					PCI_ERR_UNC_UNX_COMP |
>	\
>> +					PCI_ERR_UNC_UNSUP)
>> +
>>  static int pcie_aer_disable;
>>  static pci_ers_result_t aer_root_reset(struct pci_dev *dev);
>>
>> @@ -1196,6 +1202,41 @@ void aer_recover_queue(int domain, unsigned
>int bus, unsigned int devfn,
>>  EXPORT_SYMBOL_GPL(aer_recover_queue);
>>  #endif
>>
>> +static void anfe_get_uc_status(struct pci_dev *dev, struct aer_err_info
>*info)
>> +{
>> +	u32 uncor_mask, uncor_status;
>> +	u16 device_status;
>> +	int aer = dev->aer_cap;
>> +
>> +	if (pcie_capability_read_word(dev, PCI_EXP_DEVSTA,
>&device_status))
>> +		return;
>> +	/*
>> +	 * Take the most conservative route here. If there are
>> +	 * Non-Fatal/Fatal errors detected, do not assume any
>> +	 * bit in uncor_status is set by ANFE.
>> +	 */
>> +	if (device_status & (PCI_EXP_DEVSTA_NFED | PCI_EXP_DEVSTA_FED))
>> +		return;
>> +
>
>Is there not a race here?  If we happen to get either an NFED or FED
>between the read of device_status above and here we might pick up a status
>that corresponds to that (and hence clear something we should not).

In this scenario, info->anfe_status is 0.

>
>Or am I missing that race being close somewhere?

The bits leading to NFED or FED is masked out when assigning info->anfe_status.
Bits for FED is masked out by ~info->severity,
bit for NFED is masked out by AER_ERR_ANFE_UNC_MASK.

So we never clear status bits for NFED or FED in ANFE handler.

See below assignment of info->anfe_status.

Thanks
Zhenzhong

>
>> +	pci_read_config_dword(dev, aer + PCI_ERR_UNCOR_STATUS,
>&uncor_status);
>> +	pci_read_config_dword(dev, aer + PCI_ERR_UNCOR_MASK,
>&uncor_mask);
>> +	/*
>> +	 * According to PCIe Base Specification Revision 6.1,
>> +	 * Section 6.2.3.2.4, if an UNCOR error is raised as
>> +	 * Advisory Non-Fatal error, it will match the following
>> +	 * conditions:
>> +	 *	a. The severity of the error is Non-Fatal.
>> +	 *	b. The error is one of the following:
>> +	 *		1. Poisoned TLP           (Section 6.2.3.2.4.3)
>> +	 *		2. Completion Timeout     (Section 6.2.3.2.4.4)
>> +	 *		3. Completer Abort        (Section 6.2.3.2.4.1)
>> +	 *		4. Unexpected Completion  (Section 6.2.3.2.4.5)
>> +	 *		5. Unsupported Request    (Section 6.2.3.2.4.1)
>> +	 */
>> +	info->anfe_status = uncor_status & ~uncor_mask & ~info->severity
>&
>> +			    AER_ERR_ANFE_UNC_MASK;
>> +}
>> +
>>  /**
>>   * aer_get_device_error_info - read error status from dev and store it to
>info
>>   * @dev: pointer to the device expected to have a error record
>> @@ -1213,6 +1254,7 @@ int aer_get_device_error_info(struct pci_dev
>*dev, struct aer_err_info *info)
>>
>>  	/* Must reset in this function */
>>  	info->status = 0;
>> +	info->anfe_status = 0;
>>  	info->tlp_header_valid = 0;
>>
>>  	/* The device might not support AER */
>> @@ -1226,6 +1268,9 @@ int aer_get_device_error_info(struct pci_dev
>*dev, struct aer_err_info *info)
>>  			&info->mask);
>>  		if (!(info->status & ~info->mask))
>>  			return 0;
>> +
>> +		if (info->status & PCI_ERR_COR_ADV_NFAT)
>> +			anfe_get_uc_status(dev, info);
>>  	} else if (type == PCI_EXP_TYPE_ROOT_PORT ||
>>  		   type == PCI_EXP_TYPE_RC_EC ||
>>  		   type == PCI_EXP_TYPE_DOWNSTREAM ||


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3 1/3] PCI/AER: Store UNCOR_STATUS bits that might be ANFE in aer_err_info
  2024-04-23  2:25     ` Duan, Zhenzhong
@ 2024-04-26 16:11       ` Jonathan Cameron
  2024-04-28  3:31         ` Duan, Zhenzhong
  0 siblings, 1 reply; 8+ messages in thread
From: Jonathan Cameron @ 2024-04-26 16:11 UTC (permalink / raw)
  To: Duan, Zhenzhong
  Cc: linux-pci, linuxppc-dev, linux-acpi, rafael, lenb, james.morse,
	Luck, Tony, bp, dave, Jiang, Dave, Schofield, Alison, Verma,
	Vishal L, Weiny, Ira, bhelgaas, helgaas, mahesh, oohall,
	linmiaohe, shiju.jose, Preble, Adam C, leoyang.li, lukas,
	Smita.KoralahalliChannabasappa, rrichter, linux-cxl, linux-edac,
	linux-kernel, Tsaur, Erwin, Kuppuswamy, Sathyanarayanan,
	Williams, Dan J, Wanyan, Feiting, Wang, Yudong, Peng, Chao P,
	qingshun.wang

On Tue, 23 Apr 2024 02:25:05 +0000
"Duan, Zhenzhong" <zhenzhong.duan@intel.com> wrote:

> >-----Original Message-----
> >From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> >Subject: Re: [PATCH v3 1/3] PCI/AER: Store UNCOR_STATUS bits that might
> >be ANFE in aer_err_info
> >
> >On Wed, 17 Apr 2024 14:14:05 +0800
> >Zhenzhong Duan <zhenzhong.duan@intel.com> wrote:
> >  
> >> In some cases the detector of a Non-Fatal Error(NFE) is not the most
> >> appropriate agent to determine the type of the error. For example,
> >> when software performs a configuration read from a non-existent
> >> device or Function, completer will send an ERR_NONFATAL Message.
> >> On some platforms, ERR_NONFATAL results in a System Error, which
> >> breaks normal software probing.
> >>
> >> Advisory Non-Fatal Error(ANFE) is a special case that can be used
> >> in above scenario. It is predominantly determined by the role of the
> >> detecting agent (Requester, Completer, or Receiver) and the specific
> >> error. In such cases, an agent with AER signals the NFE (if enabled)
> >> by sending an ERR_COR Message as an advisory to software, instead of
> >> sending ERR_NONFATAL.
> >>
> >> When processing an ANFE, ideally both correctable error(CE) status and
> >> uncorrectable error(UE) status should be cleared. However, there is no
> >> way to fully identify the UE associated with ANFE. Even worse, a Fatal
> >> Error(FE) or Non-Fatal Error(NFE) may set the same UE status bit as
> >> ANFE. Treating an ANFE as NFE will reproduce above mentioned issue,
> >> i.e., breaking softwore probing; treating NFE as ANFE will make us
> >> ignoring some UEs which need active recover operation. To avoid clearing
> >> UEs that are not ANFE by accident, the most conservative route is taken
> >> here: If any of the FE/NFE Detected bits is set in Device Status, do not
> >> touch UE status, they should be cleared later by the UE handler. Otherwise,
> >> a specific set of UEs that may be raised as ANFE according to the PCIe
> >> specification will be cleared if their corresponding severity is Non-Fatal.
> >>
> >> To achieve above purpose, store UNCOR_STATUS bits that might be ANFE
> >> in aer_err_info.anfe_status. So that those bits could be printed and
> >> processed later.
> >>
> >> Tested-by: Yudong Wang <yudong.wang@intel.com>
> >> Co-developed-by: "Wang, Qingshun" <qingshun.wang@linux.intel.com>
> >> Signed-off-by: "Wang, Qingshun" <qingshun.wang@linux.intel.com>
> >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> >> ---
> >>  drivers/pci/pci.h      |  1 +
> >>  drivers/pci/pcie/aer.c | 45  
> >++++++++++++++++++++++++++++++++++++++++++  
> >>  2 files changed, 46 insertions(+)
> >>
> >> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> >> index 17fed1846847..3f9eb807f9fd 100644
> >> --- a/drivers/pci/pci.h
> >> +++ b/drivers/pci/pci.h
> >> @@ -412,6 +412,7 @@ struct aer_err_info {
> >>
> >>  	unsigned int status;		/* COR/UNCOR Error Status */
> >>  	unsigned int mask;		/* COR/UNCOR Error Mask */
> >> +	unsigned int anfe_status;	/* UNCOR Error Status for ANFE */
> >>  	struct pcie_tlp_log tlp;	/* TLP Header */
> >>  };
> >>
> >> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> >> index ac6293c24976..27364ab4b148 100644
> >> --- a/drivers/pci/pcie/aer.c
> >> +++ b/drivers/pci/pcie/aer.c
> >> @@ -107,6 +107,12 @@ struct aer_stats {
> >>  					PCI_ERR_ROOT_MULTI_COR_RCV |  
> >	\  
> >>  					PCI_ERR_ROOT_MULTI_UNCOR_RCV)
> >>
> >> +#define AER_ERR_ANFE_UNC_MASK  
> >	(PCI_ERR_UNC_POISON_TLP |	\  
> >> +					PCI_ERR_UNC_COMP_TIME |  
> >	\  
> >> +					PCI_ERR_UNC_COMP_ABORT |  
> >	\  
> >> +					PCI_ERR_UNC_UNX_COMP |  
> >	\  
> >> +					PCI_ERR_UNC_UNSUP)
> >> +
> >>  static int pcie_aer_disable;
> >>  static pci_ers_result_t aer_root_reset(struct pci_dev *dev);
> >>
> >> @@ -1196,6 +1202,41 @@ void aer_recover_queue(int domain, unsigned  
> >int bus, unsigned int devfn,  
> >>  EXPORT_SYMBOL_GPL(aer_recover_queue);
> >>  #endif
> >>
> >> +static void anfe_get_uc_status(struct pci_dev *dev, struct aer_err_info  
> >*info)  
> >> +{
> >> +	u32 uncor_mask, uncor_status;
> >> +	u16 device_status;
> >> +	int aer = dev->aer_cap;
> >> +
> >> +	if (pcie_capability_read_word(dev, PCI_EXP_DEVSTA,  
> >&device_status))  
> >> +		return;
> >> +	/*
> >> +	 * Take the most conservative route here. If there are
> >> +	 * Non-Fatal/Fatal errors detected, do not assume any
> >> +	 * bit in uncor_status is set by ANFE.
> >> +	 */
> >> +	if (device_status & (PCI_EXP_DEVSTA_NFED | PCI_EXP_DEVSTA_FED))
> >> +		return;
> >> +  
> >
> >Is there not a race here?  If we happen to get either an NFED or FED
> >between the read of device_status above and here we might pick up a status
> >that corresponds to that (and hence clear something we should not).  
> 
> In this scenario, info->anfe_status is 0.

OK. In that case what is the point of the check above?
If the code is safe to races, it's safe to go ahead without that check
on what might race.

> 
> >
> >Or am I missing that race being close somewhere?  
> 
> The bits leading to NFED or FED is masked out when assigning info->anfe_status.
> Bits for FED is masked out by ~info->severity,
> bit for NFED is masked out by AER_ERR_ANFE_UNC_MASK.
> 
> So we never clear status bits for NFED or FED in ANFE handler.
> 
> See below assignment of info->anfe_status.
> 
> Thanks
> Zhenzhong
> 
> >  
> >> +	pci_read_config_dword(dev, aer + PCI_ERR_UNCOR_STATUS,  
> >&uncor_status);  
> >> +	pci_read_config_dword(dev, aer + PCI_ERR_UNCOR_MASK,  
> >&uncor_mask);  
> >> +	/*
> >> +	 * According to PCIe Base Specification Revision 6.1,
> >> +	 * Section 6.2.3.2.4, if an UNCOR error is raised as
> >> +	 * Advisory Non-Fatal error, it will match the following
> >> +	 * conditions:
> >> +	 *	a. The severity of the error is Non-Fatal.
> >> +	 *	b. The error is one of the following:
> >> +	 *		1. Poisoned TLP           (Section 6.2.3.2.4.3)
> >> +	 *		2. Completion Timeout     (Section 6.2.3.2.4.4)
> >> +	 *		3. Completer Abort        (Section 6.2.3.2.4.1)
> >> +	 *		4. Unexpected Completion  (Section 6.2.3.2.4.5)
> >> +	 *		5. Unsupported Request    (Section 6.2.3.2.4.1)
> >> +	 */
> >> +	info->anfe_status = uncor_status & ~uncor_mask & ~info->severity  
> >&  
> >> +			    AER_ERR_ANFE_UNC_MASK;
> >> +}
> >> +
> >>  /**
> >>   * aer_get_device_error_info - read error status from dev and store it to  
> >info  
> >>   * @dev: pointer to the device expected to have a error record
> >> @@ -1213,6 +1254,7 @@ int aer_get_device_error_info(struct pci_dev  
> >*dev, struct aer_err_info *info)  
> >>
> >>  	/* Must reset in this function */
> >>  	info->status = 0;
> >> +	info->anfe_status = 0;
> >>  	info->tlp_header_valid = 0;
> >>
> >>  	/* The device might not support AER */
> >> @@ -1226,6 +1268,9 @@ int aer_get_device_error_info(struct pci_dev  
> >*dev, struct aer_err_info *info)  
> >>  			&info->mask);
> >>  		if (!(info->status & ~info->mask))
> >>  			return 0;
> >> +
> >> +		if (info->status & PCI_ERR_COR_ADV_NFAT)
> >> +			anfe_get_uc_status(dev, info);
> >>  	} else if (type == PCI_EXP_TYPE_ROOT_PORT ||
> >>  		   type == PCI_EXP_TYPE_RC_EC ||
> >>  		   type == PCI_EXP_TYPE_DOWNSTREAM ||  
> 
> 


^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH v3 1/3] PCI/AER: Store UNCOR_STATUS bits that might be ANFE in aer_err_info
  2024-04-26 16:11       ` Jonathan Cameron
@ 2024-04-28  3:31         ` Duan, Zhenzhong
  0 siblings, 0 replies; 8+ messages in thread
From: Duan, Zhenzhong @ 2024-04-28  3:31 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: linux-pci, linuxppc-dev, linux-acpi, rafael, lenb, james.morse,
	Luck, Tony, bp, dave, Jiang, Dave, Schofield, Alison, Verma,
	Vishal L, Weiny, Ira, bhelgaas, helgaas, mahesh, oohall,
	linmiaohe, shiju.jose, Preble, Adam C, leoyang.li, lukas,
	Smita.KoralahalliChannabasappa, rrichter, linux-cxl, linux-edac,
	linux-kernel, Tsaur, Erwin, Kuppuswamy, Sathyanarayanan,
	Williams, Dan J, Wanyan, Feiting, Wang, Yudong, Peng, Chao P,
	qingshun.wang

Hi Jonathan,

>-----Original Message-----
>From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
>Subject: Re: [PATCH v3 1/3] PCI/AER: Store UNCOR_STATUS bits that might
>be ANFE in aer_err_info
>
>On Tue, 23 Apr 2024 02:25:05 +0000
>"Duan, Zhenzhong" <zhenzhong.duan@intel.com> wrote:
>
>> >-----Original Message-----
>> >From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
>> >Subject: Re: [PATCH v3 1/3] PCI/AER: Store UNCOR_STATUS bits that
>might
>> >be ANFE in aer_err_info
>> >
>> >On Wed, 17 Apr 2024 14:14:05 +0800
>> >Zhenzhong Duan <zhenzhong.duan@intel.com> wrote:
>> >
>> >> In some cases the detector of a Non-Fatal Error(NFE) is not the most
>> >> appropriate agent to determine the type of the error. For example,
>> >> when software performs a configuration read from a non-existent
>> >> device or Function, completer will send an ERR_NONFATAL Message.
>> >> On some platforms, ERR_NONFATAL results in a System Error, which
>> >> breaks normal software probing.
>> >>
>> >> Advisory Non-Fatal Error(ANFE) is a special case that can be used
>> >> in above scenario. It is predominantly determined by the role of the
>> >> detecting agent (Requester, Completer, or Receiver) and the specific
>> >> error. In such cases, an agent with AER signals the NFE (if enabled)
>> >> by sending an ERR_COR Message as an advisory to software, instead of
>> >> sending ERR_NONFATAL.
>> >>
>> >> When processing an ANFE, ideally both correctable error(CE) status and
>> >> uncorrectable error(UE) status should be cleared. However, there is no
>> >> way to fully identify the UE associated with ANFE. Even worse, a Fatal
>> >> Error(FE) or Non-Fatal Error(NFE) may set the same UE status bit as
>> >> ANFE. Treating an ANFE as NFE will reproduce above mentioned issue,
>> >> i.e., breaking softwore probing; treating NFE as ANFE will make us
>> >> ignoring some UEs which need active recover operation. To avoid
>clearing
>> >> UEs that are not ANFE by accident, the most conservative route is taken
>> >> here: If any of the FE/NFE Detected bits is set in Device Status, do not
>> >> touch UE status, they should be cleared later by the UE handler.
>Otherwise,
>> >> a specific set of UEs that may be raised as ANFE according to the PCIe
>> >> specification will be cleared if their corresponding severity is Non-Fatal.
>> >>
>> >> To achieve above purpose, store UNCOR_STATUS bits that might be
>ANFE
>> >> in aer_err_info.anfe_status. So that those bits could be printed and
>> >> processed later.
>> >>
>> >> Tested-by: Yudong Wang <yudong.wang@intel.com>
>> >> Co-developed-by: "Wang, Qingshun" <qingshun.wang@linux.intel.com>
>> >> Signed-off-by: "Wang, Qingshun" <qingshun.wang@linux.intel.com>
>> >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> >> ---
>> >>  drivers/pci/pci.h      |  1 +
>> >>  drivers/pci/pcie/aer.c | 45
>> >++++++++++++++++++++++++++++++++++++++++++
>> >>  2 files changed, 46 insertions(+)
>> >>
>> >> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>> >> index 17fed1846847..3f9eb807f9fd 100644
>> >> --- a/drivers/pci/pci.h
>> >> +++ b/drivers/pci/pci.h
>> >> @@ -412,6 +412,7 @@ struct aer_err_info {
>> >>
>> >>  	unsigned int status;		/* COR/UNCOR Error Status */
>> >>  	unsigned int mask;		/* COR/UNCOR Error Mask */
>> >> +	unsigned int anfe_status;	/* UNCOR Error Status for ANFE */
>> >>  	struct pcie_tlp_log tlp;	/* TLP Header */
>> >>  };
>> >>
>> >> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
>> >> index ac6293c24976..27364ab4b148 100644
>> >> --- a/drivers/pci/pcie/aer.c
>> >> +++ b/drivers/pci/pcie/aer.c
>> >> @@ -107,6 +107,12 @@ struct aer_stats {
>> >>  					PCI_ERR_ROOT_MULTI_COR_RCV |
>> >	\
>> >>  					PCI_ERR_ROOT_MULTI_UNCOR_RCV)
>> >>
>> >> +#define AER_ERR_ANFE_UNC_MASK
>> >	(PCI_ERR_UNC_POISON_TLP |	\
>> >> +					PCI_ERR_UNC_COMP_TIME |
>> >	\
>> >> +					PCI_ERR_UNC_COMP_ABORT |
>> >	\
>> >> +					PCI_ERR_UNC_UNX_COMP |
>> >	\
>> >> +					PCI_ERR_UNC_UNSUP)
>> >> +
>> >>  static int pcie_aer_disable;
>> >>  static pci_ers_result_t aer_root_reset(struct pci_dev *dev);
>> >>
>> >> @@ -1196,6 +1202,41 @@ void aer_recover_queue(int domain,
>unsigned
>> >int bus, unsigned int devfn,
>> >>  EXPORT_SYMBOL_GPL(aer_recover_queue);
>> >>  #endif
>> >>
>> >> +static void anfe_get_uc_status(struct pci_dev *dev, struct
>aer_err_info
>> >*info)
>> >> +{
>> >> +	u32 uncor_mask, uncor_status;
>> >> +	u16 device_status;
>> >> +	int aer = dev->aer_cap;
>> >> +
>> >> +	if (pcie_capability_read_word(dev, PCI_EXP_DEVSTA,
>> >&device_status))
>> >> +		return;
>> >> +	/*
>> >> +	 * Take the most conservative route here. If there are
>> >> +	 * Non-Fatal/Fatal errors detected, do not assume any
>> >> +	 * bit in uncor_status is set by ANFE.
>> >> +	 */
>> >> +	if (device_status & (PCI_EXP_DEVSTA_NFED | PCI_EXP_DEVSTA_FED))
>> >> +		return;
>> >> +
>> >
>> >Is there not a race here?  If we happen to get either an NFED or FED
>> >between the read of device_status above and here we might pick up a
>status
>> >that corresponds to that (and hence clear something we should not).
>>
>> In this scenario, info->anfe_status is 0.
>
>OK. In that case what is the point of the check above?
>If the code is safe to races, it's safe to go ahead without that check
>on what might race.

Good question.
After further digging into the spec, I just found I misunderstood it.
An UNCUR error raised as ANFE can be raised as NFE in different cases,
so info->anfe_status can be nonzero here and the race you mentioned
does exist, the check on PCI_EXP_DEVSTA_FED is also unnecessary.
Sorry for the misleading. I plan to have below change to fix the race:

       unsigned int anfe_status;
       anfe_status = uncor_status & ~uncor_mask & ~info->severity &
                           AER_ERR_ANFE_UNC_MASK;

       if (pcie_capability_read_word(dev, PCI_EXP_DEVSTA, &device_status))
               return;
       /*
        * Take the most conservative route here. If there are
        * Non-Fatal errors detected, do not assume any
        * bit in uncor_status is set by ANFE.
        */
       if (device_status & PCI_EXP_DEVSTA_NFED)
               return;
        info->anfe_status = anfe_status;

With this change, there is still a small window between reading uncor_status
and device_status to leak ANFE, but that's the best we can do and better
than clearing NFE. Let me know if you have better idea😊

Thanks
Zhenzhong

>
>>
>> >
>> >Or am I missing that race being close somewhere?
>>
>> The bits leading to NFED or FED is masked out when assigning info-
>>anfe_status.
>> Bits for FED is masked out by ~info->severity,
>> bit for NFED is masked out by AER_ERR_ANFE_UNC_MASK.
>>
>> So we never clear status bits for NFED or FED in ANFE handler.
>>
>> See below assignment of info->anfe_status.
>>
>> Thanks
>> Zhenzhong
>>
>> >
>> >> +	pci_read_config_dword(dev, aer + PCI_ERR_UNCOR_STATUS,
>> >&uncor_status);
>> >> +	pci_read_config_dword(dev, aer + PCI_ERR_UNCOR_MASK,
>> >&uncor_mask);
>> >> +	/*
>> >> +	 * According to PCIe Base Specification Revision 6.1,
>> >> +	 * Section 6.2.3.2.4, if an UNCOR error is raised as
>> >> +	 * Advisory Non-Fatal error, it will match the following
>> >> +	 * conditions:
>> >> +	 *	a. The severity of the error is Non-Fatal.
>> >> +	 *	b. The error is one of the following:
>> >> +	 *		1. Poisoned TLP           (Section 6.2.3.2.4.3)
>> >> +	 *		2. Completion Timeout     (Section 6.2.3.2.4.4)
>> >> +	 *		3. Completer Abort        (Section 6.2.3.2.4.1)
>> >> +	 *		4. Unexpected Completion  (Section 6.2.3.2.4.5)
>> >> +	 *		5. Unsupported Request    (Section 6.2.3.2.4.1)
>> >> +	 */
>> >> +	info->anfe_status = uncor_status & ~uncor_mask & ~info->severity
>> >&
>> >> +			    AER_ERR_ANFE_UNC_MASK;
>> >> +}
>> >> +
>> >>  /**
>> >>   * aer_get_device_error_info - read error status from dev and store it
>to
>> >info
>> >>   * @dev: pointer to the device expected to have a error record
>> >> @@ -1213,6 +1254,7 @@ int aer_get_device_error_info(struct pci_dev
>> >*dev, struct aer_err_info *info)
>> >>
>> >>  	/* Must reset in this function */
>> >>  	info->status = 0;
>> >> +	info->anfe_status = 0;
>> >>  	info->tlp_header_valid = 0;
>> >>
>> >>  	/* The device might not support AER */
>> >> @@ -1226,6 +1268,9 @@ int aer_get_device_error_info(struct pci_dev
>> >*dev, struct aer_err_info *info)
>> >>  			&info->mask);
>> >>  		if (!(info->status & ~info->mask))
>> >>  			return 0;
>> >> +
>> >> +		if (info->status & PCI_ERR_COR_ADV_NFAT)
>> >> +			anfe_get_uc_status(dev, info);
>> >>  	} else if (type == PCI_EXP_TYPE_ROOT_PORT ||
>> >>  		   type == PCI_EXP_TYPE_RC_EC ||
>> >>  		   type == PCI_EXP_TYPE_DOWNSTREAM ||
>>
>>


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2024-04-28  3:31 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-17  6:14 [PATCH v3 0/3] PCI/AER: Handle Advisory Non-Fatal error Zhenzhong Duan
2024-04-17  6:14 ` [PATCH v3 1/3] PCI/AER: Store UNCOR_STATUS bits that might be ANFE in aer_err_info Zhenzhong Duan
2024-04-22 16:16   ` Jonathan Cameron
2024-04-23  2:25     ` Duan, Zhenzhong
2024-04-26 16:11       ` Jonathan Cameron
2024-04-28  3:31         ` Duan, Zhenzhong
2024-04-17  6:14 ` [PATCH v3 2/3] PCI/AER: Print UNCOR_STATUS bits that might be ANFE Zhenzhong Duan
2024-04-17  6:14 ` [PATCH v3 3/3] PCI/AER: Clear " Zhenzhong Duan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).