Re: [PATCH v6] cxl: add RAS status unmasking for CXL

[Bjorn Helgaas <helgaas@kernel.org>]

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@Huawei.com/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?

> +		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 ;)

> +		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?

> +++ 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.

Bjorn