Messages in this thread | | | Date | Fri, 16 Jun 2023 11:28:01 -0500 | Subject | Re: [PATCH v5 24/26] cxl/pci: Add RCH downstream port error logging | From | Terry Bowman <> |
| |
Hi Dan,
On 6/16/23 11:17, Terry Bowman wrote: > Hi Dan, > > I added responses below. > > On 6/12/23 16:38, Dan Williams wrote: >> 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@amd.com> >>> Signed-off-by: Robert Richter <rrichter@amd.com> >>> Signed-off-by: Terry Bowman <terry.bowman@amd.com> >>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.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. >> > > > I'll take a look at this. We have most of the patchset requests implplemented > and will give me time to look at this. > >>> + >>> +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. >> > > Ok. > >>> + >>> + if (!cxl_pci_find_port(pdev, &dport) || !dport->rch) >>> + return; >> >> The reference for the @port return from cxl_pci_find_port() is leaked >> here. >>
I will address this as well. Thanks for pointing this out.
Regards, Terry
| |