Message-ID: <20221027213010.GA827560@bhelgaas>
Date:   Thu, 27 Oct 2022 16:30:10 -0500
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Terry Bowman <terry.bowman@....com>
Cc:     alison.schofield@...el.com, vishal.l.verma@...el.com,
        dave.jiang@...el.com, ira.weiny@...el.com, bwidawsk@...nel.org,
        dan.j.williams@...el.com, linux-cxl@...r.kernel.org,
        linux-kernel@...r.kernel.org, bhelgaas@...gle.com,
        rafael@...nel.org, lenb@...nel.org, Jonathan.Cameron@...wei.com,
        dave@...olabs.net, rrichter@....com
Subject: Re: [PATCH 5/5] cxl/pci: Log CXL device's PCIe AER and CXL RAS error
 information

On Fri, Oct 21, 2022 at 01:56:15PM -0500, Terry Bowman wrote:
> The CXL downport PCIe AER and CXL RAS capability information needs to be
> logged during PCIe AER error handling.
> 
> The existing PCIe AER error handler logs native AER errors but does not
> log upport/downport AER capability residing in the RCRB. The CXL1.1
> RCRB does not have a BDF and is not enunmerable. The existing error handler
> logic does not display CXL RAS details either.

s/enunmerable/enumerable/

The patch itself doesn't seem to reference RCRB.  What's the
connection?

Is this specific to CXL?  The base PCIe spec also documents an RCRB,
though I don't think Linux does anything with it.

I guess at least the RCRB discovery must be CXL-specific, since I have
no idea how to find a generic PCIe RCRB.

> +static void cxl_error_report(struct cxl_memdev *cxlmd)
> +{
> +	struct pci_dev *pdev = to_pci_dev(cxlmd->cxlds->dev);
> +	struct aer_capability_regs *aer_cap;
> +	struct ras_cap *ras_cap;
> +
> +	aer_cap = (struct aer_capability_regs *)cxlmd->cxlds->aer_map.base;
> +	ras_cap = (struct ras_cap *)cxlmd->cxlds->ras_map.base;

I don't think you need casts since .base is void *.

> +	pci_err(pdev, "CXL Error Report\n");
> +	pci_err(pdev, "AER Errors:\n");
> +	if (aer_cap) {
> +		cxl_print_aer(pdev, AER_CORRECTABLE, aer_cap);
> +		cxl_print_aer(pdev, AER_NONFATAL, aer_cap);
> +		cxl_print_aer(pdev, AER_FATAL, aer_cap);
> +	}
> +
> +	pci_err(pdev, "RAS Errors:\n");
> +	if (ras_cap) {
> +		pci_err(pdev, "RAS: uc_error_status = %X\n", readl(&ras_cap->uc_error_status));

"%X" will look a lot different than what cper_print_aer() logged
above.  No "0x", upper-case vs lower-case, "=" vs ":", etc.  Maybe
there should be a hint to connect RAS with CXL (maybe there's already
a dev_fmt somewhere that I missed)?

> +static void cxl_error_detected(struct pci_dev *pdev)
> +{
> +	struct cxl_memdev *cxlmd;
> +
> +	if (!is_cxl_memdev(&pdev->dev)) {
> +		pci_err(pdev, "CXL memory device is invalid\n");
> +		return;
> +	}
> +
> +	cxlmd = dev_get_drvdata(&pdev->dev);
> +	if (!cxlmd) {
> +		pci_err(pdev, "CXL memory device is NULL\n");
> +		return;
> +	}
> +
> +	if (!cxlmd->cxlds) {
> +		pci_err(pdev, "CXL device state object is NULL\n");
> +		return;
> +	}

Would these NULL pointers indicate a programming error, or do they
indicate lack of an optional feature?  If the former, I generally
prefer to just take the NULL pointer dereference oops instead of just
printing an easily-missed message.  But maybe the CXL style is to be
more defensive.

> +void cxl_print_aer(struct pci_dev *dev, int aer_severity,
> +		    struct aer_capability_regs *aer)
> +{
> +	cper_print_aer(dev, aer_severity, aer);

What is the purpose of this wrapper?  I guess you need an exported
symbol for some reason?

> +}
> +EXPORT_SYMBOL_GPL(cxl_print_aer);

> +static void report_cxl_errors(struct aer_rpc *rpc,
> +			      struct aer_err_source *e_src)
> +{
> +	struct pci_dev *pdev = rpc->rpd;
> +	struct aer_err_info e_info;
> +	u32 uncor_status, cor_status;
> +
> +	pci_read_config_dword(pdev, pdev->aer_cap + PCI_ERR_UNCOR_STATUS, &uncor_status);
> +	pci_read_config_dword(pdev, pdev->aer_cap + PCI_ERR_COR_STATUS, &cor_status);

I think it's kind of an existing defect that we don't have a single
place to read these registers.  I think they should be read either in
firmware (for firmware-first error handling, where Linux basically
gets a package of these register contents) or in Linux (for native
handling).  Ideally I think these paths would converge right after
Linux reads them.

Anyway, I don't think we should read these registers *again* for CXL.
And I assume firmware-first error handling should work for CXL as well
as for base PCIe?  That would imply that we wouldn't read them at all
here for the firmware-first case.

> +	if (!uncor_status && !cor_status)
> +		return;
> +
> +	if (pci_pcie_type(pdev) == PCI_EXP_TYPE_RC_EC)
> +		pcie_walk_rcec(pdev, report_cxl_errors_iter, &e_info);
> +	else
> +		pci_walk_bus(pdev->subordinate, report_cxl_errors_iter, &e_info);
> +
> +	pci_write_config_dword(pdev, pdev->aer_cap + PCI_ERR_UNCOR_STATUS, uncor_status);
> +	pci_write_config_dword(pdev, pdev->aer_cap + PCI_ERR_COR_STATUS, cor_status);

Shouldn't this clearing be somehow contingent on pcie_aer_is_native()?

> +++ b/include/linux/pci.h
> @@ -827,6 +827,10 @@ enum pci_ers_result {
>  
>  /* PCI bus error event callbacks */
>  struct pci_error_handlers {
> +
> +	/* CXL error detected on this device */

Nit on the comment: calling this function doesn't imply that a CXL
error was detected; we *always* call it.  Apparently it's just an
opportunity to log any CXL-specific errors that may have occurred?

I think we need a comment about why this couldn't be done in the
existing .error_detected() callback.  I gather it might be related to
AER_CORRECTABLE errors, for which we don't call .error_detected()?

If the purpose is only to learn about correctable errors, maybe the
callback doesn't need to be CXL-specific and could be called at the
point where we test for AER_CORRECTABLE?

> +	void (*cxl_error_detected)(struct pci_dev *dev);
> +
>  	/* PCI bus error detected on this device */
>  	pci_ers_result_t (*error_detected)(struct pci_dev *dev,
>  					   pci_channel_state_t error);
> -- 
> 2.34.1
> 

Powered by blists - more mailing lists