On 2/10/23 3:52 PM, Bjorn Helgaas wrote:
On Fri, Feb 10, 2023 at 10:04:03AM -0700, Dave Jiang wrote:
By default the CXL RAS mask registers bits are defaulted to 1's and
suppress all error reporting. If the kernel has negotiated ownership
of error handling for CXL then unmask the mask registers by writing 0s.
PCI_EXP_AER_FLAGS moved to linux/pci.h header to expose to driver. It
allows exposure of system enabled PCI error flags for the driver to decide
which error bits to toggle. Bjorn suggested that the error enabling should
be controlled from the system policy rather than a driver level choice[1].
CXL RAS CE and UE masks are checked against PCI_EXP_AER_FLAGS before
unmasking.
[1]: https://lore.kernel.org/linux-cxl/20230210122952.00006999@xxxxxxxxxx/T/#me8c7f39d43029c64ccff5c950b78a2cee8e885af
+static int cxl_pci_ras_unmask(struct pci_dev *pdev)
+{
+ struct pci_host_bridge *host_bridge = pci_find_host_bridge(pdev->bus);
+ struct cxl_dev_state *cxlds = pci_get_drvdata(pdev);
+ void __iomem *addr;
+ u32 orig_val, val, mask;
+
+ if (!cxlds->regs.ras)
+ return -ENODEV;
+
+ /* BIOS has CXL error control */
+ if (!host_bridge->native_cxl_error)
+ return -EOPNOTSUPP;
+
+ if (PCI_EXP_AER_FLAGS & PCI_EXP_DEVCTL_URRE) {
1) I don't really want to expose PCI_EXP_AER_FLAGS in linux/pci.h.
It's basically a convenience part of the AER implementation.
2) I think your intent here is to configure the CXL RAS masking based
on what PCIe error reporting is enabled, but doing it by looking at
PCI_EXP_AER_FLAGS doesn't seem right. This expression is a
compile-time constant that is always true, but we can't rely on
devices always being configured that way.
We call pci_aer_init() for every device during enumeration, but we
only write PCI_EXP_AER_FLAGS if pci_aer_available() and if
pcie_aer_is_native(). And there are a bunch of drivers that call
pci_disable_pcie_error_reporting(), which *clears* those flags. I'm
not sure those drivers *should* be doing that, but they do today.
I'm not sure why this needs to be conditional at all, but if it does,
maybe you want to read PCI_EXP_DEVCTL and base it on that?
Ok I'll read the PCI_EXP_DEVCTL. Looking to only unmask the relevant RAS
reporting if respective PCIe bits are enabled.
+ addr = cxlds->regs.ras + CXL_RAS_UNCORRECTABLE_MASK_OFFSET;
+ orig_val = readl(addr);
+
+ mask = CXL_RAS_UNCORRECTABLE_MASK_MASK;
Weird name ("_MASK_MASK"), but I assume there's a good reason ;)
Yes. It's the mask of the error mask register. Is that too much of a
mouthful? I can take out the second mask.
+ if (!cxl_pci_flit_256(pdev))
+ mask &= ~CXL_RAS_UNCORRECTABLE_MASK_F256B_MASK;
+ val = orig_val & ~mask;
+ writel(val, addr);
+ dev_dbg(&pdev->dev,
+ "Uncorrectable RAS Errors Mask: %#x -> %#x\n",
+ orig_val, val);
+ }
if (cxlds->regs.ras) {
- pci_enable_pcie_error_reporting(pdev);
- rc = devm_add_action_or_reset(&pdev->dev, disable_aer, pdev);
- if (rc)
- return rc;
+ rc = pci_enable_pcie_error_reporting(pdev);
I see you're just adding a check of return value here, but I'm not
sure you need to call pci_enable_pcie_error_reporting() in the first
place. Isn't the call in the pci_aer_init() path enough?
I guess I'm confused by the kernel documentation:
"
pci_enable_pcie_error_reporting enables the device to send error
messages to root port when an error is detected. Note that devices
don't enable the error reporting by default, so device drivers need
call this function to enable it.
"
Seems to indicate that driver should always call this if it wants AER
reporting?
+++ b/include/uapi/linux/pci_regs.h
@@ -693,6 +693,7 @@
#define PCI_EXP_LNKCTL2_TX_MARGIN 0x0380 /* Transmit Margin */
#define PCI_EXP_LNKCTL2_HASD 0x0020 /* HW Autonomous Speed Disable */
#define PCI_EXP_LNKSTA2 0x32 /* Link Status 2 */
+#define PCI_EXP_LNKSTA2_FLIT BIT(10) /* Flit Mode Status */
Please spell out the hex constant. This is to match the style of the
surrounding code, and it also gives a hint about the size of the
register.
Ok will fix
Bjorn
