On Thu, 13 Apr 2023 15:38:07 +0200
Robert Richter [off-list ref] wrote:

On 12.04.23 16:29:01, Bjorn Helgaas wrote:

quoted

On Tue, Apr 11, 2023 at 01:03:02PM -0500, Terry Bowman wrote:  

quoted

From: Robert Richter <redacted>

RCEC AER corrected and uncorrectable internal errors (CIE/UIE) are
disabled by default.  

"Disabled by default" just means "the power-up state of CIE/UIC is
that they are masked", right?  It doesn't mean that Linux normally
masks them.  

Yes, will change the wording here.

quoted

quoted

[1][2] Enable them to receive CXL downstream port
errors of a Restricted CXL Host (RCH).

[1] CXL 3.0 Spec, 12.2.1.1 - RCH Downstream Port Detected Errors
[2] PCIe Base Spec 6.0, 7.8.4.3 Uncorrectable Error Mask Register,
    7.8.4.6 Correctable Error Mask Register

Co-developed-by: Terry Bowman <redacted>
Signed-off-by: Robert Richter <redacted>
Signed-off-by: Terry Bowman <redacted>
Cc: "Oliver O'Halloran" <oohall@gmail.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Mahesh J Salgaonkar <mahesh@linux.ibm.com>
Cc: linuxppc-dev@lists.ozlabs.org
Cc: linux-pci@vger.kernel.org
---
 drivers/pci/pcie/aer.c | 73 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 73 insertions(+)

diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index 171a08fd8ebd..3973c731e11d 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c

@@ -1000,7 +1000,79 @@ static void cxl_handle_error(struct pci_dev *dev, struct aer_err_info *info)
 		pcie_walk_rcec(dev, cxl_handle_error_iter, info);
 }
 
+static bool cxl_error_is_native(struct pci_dev *dev)
+{
+	struct pci_host_bridge *host = pci_find_host_bridge(dev->bus);
+
+	if (pcie_ports_native)
+		return true;
+
+	return host->native_aer && host->native_cxl_error;
+}
+
+static int handles_cxl_error_iter(struct pci_dev *dev, void *data)
+{
+	int *handles_cxl = data;
+
+	*handles_cxl = is_cxl_mem_dev(dev) && cxl_error_is_native(dev);
+
+	return *handles_cxl;
+}
+
+static bool handles_cxl_errors(struct pci_dev *rcec)
+{
+	int handles_cxl = 0;
+
+	if (!rcec->aer_cap)
+		return false;
+
+	if (pci_pcie_type(rcec) == PCI_EXP_TYPE_RC_EC)
+		pcie_walk_rcec(rcec, handles_cxl_error_iter, &handles_cxl);
+
+	return !!handles_cxl;
+}
+
+static int __cxl_unmask_internal_errors(struct pci_dev *rcec)
+{
+	int aer, rc;
+	u32 mask;
+
+	/*
+	 * Internal errors are masked by default, unmask RCEC's here
+	 * PCI6.0 7.8.4.3 Uncorrectable Error Mask Register (Offset 08h)
+	 * PCI6.0 7.8.4.6 Correctable Error Mask Register (Offset 14h)
+	 */  

Unmasking internal errors doesn't have anything specific to do with
CXL, so I don't think it should have "cxl" in the function name.
Maybe something like "pci_aer_unmask_internal_errors()".  

Since it is static I renamed it to aer_unmask_internal_errors() and
also moved it to the beginning of the #ifdef block for easier later
reuse.

quoted

This also has nothing special to do with RCECs, so I think we should
refer to the device as "dev" as is typical in this file.  

Changed.

quoted

I think this needs to check pcie_aer_is_native() as is done by
pci_aer_clear_nonfatal_status() and other functions that write the AER
Capability.  

Also added the check to aer_unmask_internal_errors(). There was a
check for native_* in handles_cxl_errors() already, but only for the
pci devs of the RCEC. I added a check of the RCEC there too.

quoted

With the exception of this function, this patch looks like all CXL
code that maybe could be with other CXL code.  Would require making
pcie_walk_rcec() available outside drivers/pci, I guess.  

Even this is CXL code, it implements AER support and fits better here
around AER code. Export of pcie_walk_rcec() (and others?) is not the
main issue here. CXL drivers can come as modules and would need to
register a hook at the aer handler.  This would add even more
complexity here. In contrast, current solution just adds two functions
for enablement and handling which are empty stubs if code is disabled.

I could move that code to aer_cxl.c similar to aer_inject.c. Since the
CXL part is small compared to the remaining aer code I left it in
aer.c. Also, it is guarded by #ifdef which additionally encapsulates
it.

To throw another option in there (what Bjorn suggested IIRC for the more
general case..) 

Just enable internal errors always.  No need to know if they are CXL
or something else.

There will/might be fallout and it will be fun.

Jonathan

quoted

quoted

+	aer = rcec->aer_cap;
+	rc = pci_read_config_dword(rcec, aer + PCI_ERR_UNCOR_MASK, &mask);
+	if (rc)
+		return rc;
+	mask &= ~PCI_ERR_UNC_INTN;
+	rc = pci_write_config_dword(rcec, aer + PCI_ERR_UNCOR_MASK, mask);
+	if (rc)
+		return rc;
+
+	rc = pci_read_config_dword(rcec, aer + PCI_ERR_COR_MASK, &mask);
+	if (rc)
+		return rc;
+	mask &= ~PCI_ERR_COR_INTERNAL;
+	rc = pci_write_config_dword(rcec, aer + PCI_ERR_COR_MASK, mask);
+
+	return rc;
+}
+
+static void cxl_unmask_internal_errors(struct pci_dev *rcec)  

Also renaming this to cxl_enable_rcec() to more generalize the
function.

quoted

quoted

+{
+	if (!handles_cxl_errors(rcec))
+		return;
+
+	if (__cxl_unmask_internal_errors(rcec))
+		dev_err(&rcec->dev, "cxl: Failed to unmask internal errors");
+	else
+		dev_dbg(&rcec->dev, "cxl: Internal errors unmasked");  

I am going to change this to a pci_info() for alignment with other
messages around:

[   14.200265] pcieport 0000:40:00.3: PME: Signaling with IRQ 44
[   14.213925] pcieport 0000:40:00.3: AER: cxl: Internal errors unmasked
[   14.228413] pcieport 0000:40:00.3: AER: enabled with IRQ 44

Plus, using pci_err() instead of dev_err().

quoted

quoted

+}
+
 #else
+static inline void cxl_unmask_internal_errors(struct pci_dev *dev) { }
 static inline void cxl_handle_error(struct pci_dev *dev,
 				    struct aer_err_info *info) { }
 #endif

@@ -1397,6 +1469,7 @@ static int aer_probe(struct pcie_device *dev)
 		return status;
 	}
 
+	cxl_unmask_internal_errors(port);
 	aer_enable_rootport(rpc);
 	pci_info(port, "enabled with IRQ %d\n", dev->irq);
 	return 0;

-- 
2.34.1
  

Thanks for review,

-Robert