| Message ID | 20230607221651.2454764-25-terry.bowman@amd.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cxl/pci: Add support for RCH RAS error handling | expand |
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. > + > +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.
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. > > How can dport->rch be false while cxlds->rcd is true? Is that check > required? > I will remove the rch check. >> + >> + 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? > Yes, the names are ready for reworking. I have updated the functions to use consistent naming in the v6 patchset. >> +} >> + >> +#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. The 'if (cxlds->rcd)' will be moved here per your above request. Strictly speaking, this is still in the VH path but an improvement. This is really an endpoint path for RCH(RCD) and VH endpoints. An alternative solution we considered was using a separate RCH dport error handler but that requires further AER port driver plumbing rework (for only CXL) or changing the assigned error handlers depending on RCH-VH mode at runtime. I spent time implementing and testing these options and we found it added significant complexity for a limited use case. Regards, Terry
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
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 + +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; + + if (!cxl_pci_find_port(pdev, &dport) || !dport->rch) + return; + + 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); +} + +#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); + /* * A frozen channel indicates an impending reset which is fatal to * CXL.mem operation, and will likely crash the system. On the off