| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <648790561f7ea_1433ac294ed@dwillia2-xfh.jf.intel.com.notmuch>
Date: Mon, 12 Jun 2023 14:38:30 -0700
From: Dan Williams <dan.j.williams@...el.com>
To: Terry Bowman <terry.bowman@....com>, <alison.schofield@...el.com>,
<vishal.l.verma@...el.com>, <ira.weiny@...el.com>,
<bwidawsk@...nel.org>, <dan.j.williams@...el.com>,
<dave.jiang@...el.com>, <Jonathan.Cameron@...wei.com>,
<linux-cxl@...r.kernel.org>
CC: <terry.bowman@....com>, <rrichter@....com>,
<linux-kernel@...r.kernel.org>, <bhelgaas@...gle.com>
Subject: RE: [PATCH v5 24/26] cxl/pci: Add RCH downstream port error logging
Terry Bowman wrote:
> RCH downstream port error logging is missing in the current CXL driver. The
> missing AER and RAS error logging is needed for communicating driver error
> details to userspace. Update the driver to include PCIe AER and CXL RAS
> error logging.
>
> Add RCH downstream port error handling into the existing RCiEP handler.
> The downstream port error handler is added to the RCiEP error handler
> because the downstream port is implemented in a RCRB, is not PCI
> enumerable, and as a result is not directly accessible to the PCI AER
> root port driver. The AER root port driver calls the RCiEP handler for
> handling RCD errors and RCH downstream port protocol errors.
>
> Update existing RCiEP correctable and uncorrectable handlers to also call
> the RCH handler. The RCH handler will read the RCH AER registers, check for
> error severity, and if an error exists will log using an existing kernel
> AER trace routine. The RCH handler will also log downstream port RAS errors
> if they exist.
>
> Co-developed-by: Robert Richter <rrichter@....com>
> Signed-off-by: Robert Richter <rrichter@....com>
> Signed-off-by: Terry Bowman <terry.bowman@....com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>
> ---
> drivers/cxl/core/pci.c | 98 ++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 98 insertions(+)
>
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index def6ee5ab4f5..97886aacc64a 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -5,6 +5,7 @@
> #include <linux/delay.h>
> #include <linux/pci.h>
> #include <linux/pci-doe.h>
> +#include <linux/aer.h>
> #include <cxlpci.h>
> #include <cxlmem.h>
> #include <cxl.h>
> @@ -747,10 +748,105 @@ static bool cxl_report_and_clear(struct cxl_dev_state *cxlds)
> return __cxl_report_and_clear(cxlds, cxlds->regs.ras);
> }
>
> +#ifdef CONFIG_PCIEAER_CXL
A general reaction to the "ifdef in a .c file" style recommendation.
Maybe this section could move to a drivers/cxl/core/aer.c file, and be
optionally compiled by config in the Makefile? I.e. similar to:
cxl_core-$(CONFIG_TRACING) += trace.o
cxl_core-$(CONFIG_CXL_REGION) += region.o
...it is borderline just big enough, but I'll leave it up to you.
> +
> +static void cxl_log_correctable_ras_dport(struct cxl_dev_state *cxlds,
> + struct cxl_dport *dport)
> +{
> + return __cxl_log_correctable_ras(cxlds, dport->regs.ras);
> +}
> +
> +static bool cxl_report_and_clear_dport(struct cxl_dev_state *cxlds,
> + struct cxl_dport *dport)
> +{
> + return __cxl_report_and_clear(cxlds, dport->regs.ras);
> +}
> +
> +/*
> + * Copy the AER capability registers using 32 bit read accesses.
> + * This is necessary because RCRB AER capability is MMIO mapped. Clear the
> + * status after copying.
> + *
> + * @aer_base: base address of AER capability block in RCRB
> + * @aer_regs: destination for copying AER capability
> + */
> +static bool cxl_rch_get_aer_info(void __iomem *aer_base,
> + struct aer_capability_regs *aer_regs)
> +{
> + int read_cnt = sizeof(struct aer_capability_regs) / sizeof(u32);
> + u32 *aer_regs_buf = (u32 *)aer_regs;
> + int n;
> +
> + if (!aer_base)
> + return false;
> +
> + /* Use readl() to guarantee 32-bit accesses */
> + for (n = 0; n < read_cnt; n++)
> + aer_regs_buf[n] = readl(aer_base + n * sizeof(u32));
> +
> + writel(aer_regs->uncor_status, aer_base + PCI_ERR_UNCOR_STATUS);
> + writel(aer_regs->cor_status, aer_base + PCI_ERR_COR_STATUS);
> +
> + return true;
> +}
> +
> +/* Get AER severity. Return false if there is no error. */
> +static bool cxl_rch_get_aer_severity(struct aer_capability_regs *aer_regs,
> + int *severity)
> +{
> + if (aer_regs->uncor_status & ~aer_regs->uncor_mask) {
> + if (aer_regs->uncor_status & PCI_ERR_ROOT_FATAL_RCV)
> + *severity = AER_FATAL;
> + else
> + *severity = AER_NONFATAL;
> + return true;
> + }
> +
> + if (aer_regs->cor_status & ~aer_regs->cor_mask) {
> + *severity = AER_CORRECTABLE;
> + return true;
> + }
> +
> + return false;
> +}
> +
> +static void cxl_handle_rch_dport_errors(struct cxl_dev_state *cxlds)
> +{
> + struct pci_dev *pdev = to_pci_dev(cxlds->dev);
> + struct aer_capability_regs aer_regs;
> + struct cxl_dport *dport;
> + int severity;
> +
> + if (!cxlds->rcd)
> + return;
Small quibble, but I think this check belongs in the caller.
> +
> + if (!cxl_pci_find_port(pdev, &dport) || !dport->rch)
> + return;
The reference for the @port return from cxl_pci_find_port() is leaked
here.
How can dport->rch be false while cxlds->rcd is true? Is that check
required?
> +
> + if (!cxl_rch_get_aer_info(dport->regs.dport_aer, &aer_regs))
> + return;
> +
> + if (!cxl_rch_get_aer_severity(&aer_regs, &severity))
> + return;
> +
> + pci_print_aer(pdev, severity, &aer_regs);
> +
> + if (severity == AER_CORRECTABLE)
> + cxl_log_correctable_ras_dport(cxlds, dport);
> + else
> + cxl_report_and_clear_dport(cxlds, dport);
This is the code that made me go back and have heartburn about the
naming choices. I.e. would a casual reader assume that correctable
errors are not cleared, and that reporting is different than logging?
> +}
> +
> +#else
> +static void cxl_handle_rch_dport_errors(struct cxl_dev_state *cxlds) { }
> +#endif
> +
> void cxl_cor_error_detected(struct pci_dev *pdev)
> {
> struct cxl_dev_state *cxlds = pci_get_drvdata(pdev);
>
> + cxl_handle_rch_dport_errors(cxlds);
> +
> cxl_log_correctable_ras_endpoint(cxlds);
> }
> EXPORT_SYMBOL_NS_GPL(cxl_cor_error_detected, CXL);
> @@ -763,6 +859,8 @@ pci_ers_result_t cxl_error_detected(struct pci_dev *pdev,
> struct device *dev = &cxlmd->dev;
> bool ue;
>
> + cxl_handle_rch_dport_errors(cxlds);
Per above comment on "cxlds->rcd" conditional, it is mildly surprising
that even the VH path calls this helper.
Powered by blists - more mailing lists