lkml.org 
[lkml]   [2022]   [Oct]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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
>

\
 
 \ /
  Last update: 2022-10-27 23:31    [W:0.174 / U:1.248 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site