| Message ID | 20240527144356.246220-3-fabio.m.de.francesco@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | Make ELOG log and trace consistently with GHES | expand |
Fabio M. De Francesco wrote: > Currently, extlog_print() (ELOG) only reports CPER PCIe section (UEFI > v2.10, Appendix N.2.7) to the kernel log via print_extlog_rcd(). I think the critical detail is is that print_extlog_rcd() is only triggered when ras_userspace_consumers() returns true. The observation is that ras_userspace_consumers() hides information from the trace path when the intended purpose of it was to hide duplicate emissions to the kernel log when userspace is watching the tracepoints. Setting aside whether ras_userspace_consumers() is still a good idea or not, it is obvious that this patch as is may surprise environments that start seeing kernel error logs where the kernel was silent before. I think the path of least surprise would be to make sure that pci_print_aer() optionally skips emitting to the kernel log when not needed wanted. So perhaps first do a lead-in patch to optionally quiet the print messages in pci_print_aer() and then pass in KERN_DEBUG from the extlog_print() path. Then we can decide later what to do about ras_userspace_consumers(). > the similar ghes_do_proc() (GHES) prints to kernel log and calls > pci_print_aer() to report via the ftrace infrastructure. > > Add support to report the CPER PCIe Error section also via the ftrace > infrastructure by calling pci_print_aer() to make ELOG act consistently > with GHES. You might also want to explain a bit about the motivation for this which is that I/O Machine Check Arcitecture events may signal failing PCIe components or links. The AER event contains details on what was happening on the wire when the error was signaled. > > Cc: Dan Williams <dan.j.williams@intel.com> > Signed-off-by: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com> > --- > drivers/acpi/acpi_extlog.c | 30 ++++++++++++++++++++++++++++++ > drivers/pci/pcie/aer.c | 2 +- > include/linux/aer.h | 13 +++++++++++-- > 3 files changed, 42 insertions(+), 3 deletions(-) > > diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c > index e025ae390737..007ce96f8672 100644 > --- a/drivers/acpi/acpi_extlog.c > +++ b/drivers/acpi/acpi_extlog.c > @@ -131,6 +131,32 @@ static int print_extlog_rcd(const char *pfx, > return 1; > } > > +static void extlog_print_pcie(struct cper_sec_pcie *pcie_err, > + int severity) > +{ > + struct aer_capability_regs *aer; > + struct pci_dev *pdev; > + unsigned int devfn; > + unsigned int bus; > + int aer_severity; > + int domain; > + > + if (pcie_err->validation_bits & CPER_PCIE_VALID_DEVICE_ID && > + pcie_err->validation_bits & CPER_PCIE_VALID_AER_INFO) { > + aer_severity = cper_severity_to_aer(severity); > + aer = (struct aer_capability_regs *)pcie_err->aer_info; > + domain = pcie_err->device_id.segment; > + bus = pcie_err->device_id.bus; > + devfn = PCI_DEVFN(pcie_err->device_id.device, > + pcie_err->device_id.function); > + pdev = pci_get_domain_bus_and_slot(domain, bus, devfn); > + if (!pdev) > + return; > + pci_print_aer(pdev, aer_severity, aer); ...per above this would become: pci_print_aer(KERN_DEBUG, pdev, aer_severity, aer); [..] Rest of the changes look good to me.
On Mon, May 27, 2024 at 04:43:41PM +0200, Fabio M. De Francesco wrote: > Currently, extlog_print() (ELOG) only reports CPER PCIe section (UEFI > v2.10, Appendix N.2.7) to the kernel log via print_extlog_rcd(). Instead, > the similar ghes_do_proc() (GHES) prints to kernel log and calls > pci_print_aer() to report via the ftrace infrastructure. > > Add support to report the CPER PCIe Error section also via the ftrace > infrastructure by calling pci_print_aer() to make ELOG act consistently > with GHES. > > Cc: Dan Williams <dan.j.williams@intel.com> > Signed-off-by: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com> > --- > drivers/acpi/acpi_extlog.c | 30 ++++++++++++++++++++++++++++++ > drivers/pci/pcie/aer.c | 2 +- > include/linux/aer.h | 13 +++++++++++-- > 3 files changed, 42 insertions(+), 3 deletions(-) > > diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c > index e025ae390737..007ce96f8672 100644 > --- a/drivers/acpi/acpi_extlog.c > +++ b/drivers/acpi/acpi_extlog.c > @@ -131,6 +131,32 @@ static int print_extlog_rcd(const char *pfx, > return 1; > } > > +static void extlog_print_pcie(struct cper_sec_pcie *pcie_err, > + int severity) > +{ > + struct aer_capability_regs *aer; > + struct pci_dev *pdev; > + unsigned int devfn; > + unsigned int bus; > + int aer_severity; > + int domain; > + > + if (pcie_err->validation_bits & CPER_PCIE_VALID_DEVICE_ID && > + pcie_err->validation_bits & CPER_PCIE_VALID_AER_INFO) { > + aer_severity = cper_severity_to_aer(severity); > + aer = (struct aer_capability_regs *)pcie_err->aer_info; > + domain = pcie_err->device_id.segment; > + bus = pcie_err->device_id.bus; > + devfn = PCI_DEVFN(pcie_err->device_id.device, > + pcie_err->device_id.function); > + pdev = pci_get_domain_bus_and_slot(domain, bus, devfn); > + if (!pdev) > + return; > + pci_print_aer(pdev, aer_severity, aer); > + pci_dev_put(pdev); > + } I'm 100% in favor of making error reporting work and look the same across GHES and ELOG. But I do have to gripe a bit... It's already unfortunate that GHES and the native AER handling are separate paths that lead to the same place (__aer_print_error()). I'm sorry that we need to add a third path that again does fundamentally the same thing. The fact that they're separate means all the design, reviewing, testing, and maintenance effort is diluted, and error handling always gets too little love in the first place. I think this is a recipe for confusion. ghes_do_proc # GHES apei_estatus_for_each_section ... if (guid_equal(sec_type, &CPER_SEC_PCIE)) ghes_handle_aer cper_severity_to_aer aer_recover_queue kfifo_in_spinlocked(&aer_recover_ring) # add to queue aer_recover_work_func # another thread kfifo_get(&aer_recover_ring) # remove from queue pci_print_aer __aer_print_error <--- aer_irq # native AER kfifo_put(&aer_fifo) # add to queue aer_isr # another thread kfifo_get(&aer_fifo) # remove from queue ... aer_isr_one_error aer_process_err_devices aer_print_error __aer_print_error <--- extlog_print # extlog (x86 only) apei_estatus_for_each_section ... if (guid_equal(sec_type, &CPER_SEC_PCIE)) extlog_print_pcie cper_severity_to_aer pci_get_domain_bus_and_slot pci_print_aer __aer_print_error <--- And we also have CXL paths that lead to __aer_print_error(), although it seems like they at least start in the native AER (and maybe GHES?) path and branch out somewhere. My head is spinning. Do I *object* to this patch? No, not really; it's a trivial change in drivers/pci/, and Rafael can add my Acked-by: Bjorn Helgaas <bhelgaas@google.com> as needed. But I am afraid we're making ourselves a maintenance headache. > +} > + > static int extlog_print(struct notifier_block *nb, unsigned long val, > void *data) > { > @@ -179,6 +205,10 @@ static int extlog_print(struct notifier_block *nb, unsigned long val, > if (gdata->error_data_length >= sizeof(*mem)) > trace_extlog_mem_event(mem, err_seq, fru_id, fru_text, > (u8)gdata->error_severity); > + } else if (guid_equal(sec_type, &CPER_SEC_PCIE)) { > + struct cper_sec_pcie *pcie_err = acpi_hest_get_payload(gdata); > + > + extlog_print_pcie(pcie_err, gdata->error_severity); > } else { > void *err = acpi_hest_get_payload(gdata); > > diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c > index ac6293c24976..794aa15527ba 100644 > --- a/drivers/pci/pcie/aer.c > +++ b/drivers/pci/pcie/aer.c > @@ -801,7 +801,7 @@ void pci_print_aer(struct pci_dev *dev, int aer_severity, > trace_aer_event(dev_name(&dev->dev), (status & ~mask), > aer_severity, tlp_header_valid, &aer->header_log); > } > -EXPORT_SYMBOL_NS_GPL(pci_print_aer, CXL); > +EXPORT_SYMBOL_GPL(pci_print_aer); > > /** > * add_error_device - list device to be handled > diff --git a/include/linux/aer.h b/include/linux/aer.h > index 4b97f38f3fcf..fbc82206045c 100644 > --- a/include/linux/aer.h > +++ b/include/linux/aer.h > @@ -42,17 +42,26 @@ int pcie_read_tlp_log(struct pci_dev *dev, int where, struct pcie_tlp_log *log); > #if defined(CONFIG_PCIEAER) > int pci_aer_clear_nonfatal_status(struct pci_dev *dev); > int pcie_aer_is_native(struct pci_dev *dev); > +void pci_print_aer(struct pci_dev *dev, int aer_severity, > + struct aer_capability_regs *aer); > #else > static inline int pci_aer_clear_nonfatal_status(struct pci_dev *dev) > { > return -EINVAL; > } > + > static inline int pcie_aer_is_native(struct pci_dev *dev) { return 0; } > +static inline void pci_print_aer(struct pci_dev *dev, int aer_severity, > + struct aer_capability_regs *aer) > +{ } > #endif > > -void pci_print_aer(struct pci_dev *dev, int aer_severity, > - struct aer_capability_regs *aer); > +#if defined(CONFIG_ACPI_APEI_PCIEAER) > int cper_severity_to_aer(int cper_severity); > +#else > +static inline int cper_severity_to_aer(int cper_severity) { return 0; } > +#endif > + > void aer_recover_queue(int domain, unsigned int bus, unsigned int devfn, > int severity, struct aer_capability_regs *aer_regs); > #endif //_AER_H_ > -- > 2.45.1 >
Dan Williams wrote: > [ add Boris ] [ actually add Boris ] Boris, see below, thoughts on deprecating acpi_extlog... > Bjorn Helgaas wrote: > > On Mon, May 27, 2024 at 04:43:41PM +0200, Fabio M. De Francesco wrote: > > > Currently, extlog_print() (ELOG) only reports CPER PCIe section (UEFI > > > v2.10, Appendix N.2.7) to the kernel log via print_extlog_rcd(). Instead, > > > the similar ghes_do_proc() (GHES) prints to kernel log and calls > > > pci_print_aer() to report via the ftrace infrastructure. > > > > > > Add support to report the CPER PCIe Error section also via the ftrace > > > infrastructure by calling pci_print_aer() to make ELOG act consistently > > > with GHES. > > > > > > Cc: Dan Williams <dan.j.williams@intel.com> > > > Signed-off-by: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com> > > > --- > > > drivers/acpi/acpi_extlog.c | 30 ++++++++++++++++++++++++++++++ > > > drivers/pci/pcie/aer.c | 2 +- > > > include/linux/aer.h | 13 +++++++++++-- > > > 3 files changed, 42 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c > > > index e025ae390737..007ce96f8672 100644 > > > --- a/drivers/acpi/acpi_extlog.c > > > +++ b/drivers/acpi/acpi_extlog.c > > > @@ -131,6 +131,32 @@ static int print_extlog_rcd(const char *pfx, > > > return 1; > > > } > > > > > > +static void extlog_print_pcie(struct cper_sec_pcie *pcie_err, > > > + int severity) > > > +{ > > > + struct aer_capability_regs *aer; > > > + struct pci_dev *pdev; > > > + unsigned int devfn; > > > + unsigned int bus; > > > + int aer_severity; > > > + int domain; > > > + > > > + if (pcie_err->validation_bits & CPER_PCIE_VALID_DEVICE_ID && > > > + pcie_err->validation_bits & CPER_PCIE_VALID_AER_INFO) { > > > + aer_severity = cper_severity_to_aer(severity); > > > + aer = (struct aer_capability_regs *)pcie_err->aer_info; > > > + domain = pcie_err->device_id.segment; > > > + bus = pcie_err->device_id.bus; > > > + devfn = PCI_DEVFN(pcie_err->device_id.device, > > > + pcie_err->device_id.function); > > > + pdev = pci_get_domain_bus_and_slot(domain, bus, devfn); > > > + if (!pdev) > > > + return; > > > + pci_print_aer(pdev, aer_severity, aer); > > > + pci_dev_put(pdev); > > > + } > > > > I'm 100% in favor of making error reporting work and look the same > > across GHES and ELOG. But I do have to gripe a bit... > > > > It's already unfortunate that GHES and the native AER handling are > > separate paths that lead to the same place (__aer_print_error()). > > > > I'm sorry that we need to add a third path that again does > > fundamentally the same thing. The fact that they're separate means > > all the design, reviewing, testing, and maintenance effort is diluted, > > and error handling always gets too little love in the first place. > > I think this is a recipe for confusion. > > > > ghes_do_proc # GHES > > apei_estatus_for_each_section > > ... > > if (guid_equal(sec_type, &CPER_SEC_PCIE)) > > ghes_handle_aer > > cper_severity_to_aer > > aer_recover_queue > > kfifo_in_spinlocked(&aer_recover_ring) # add to queue > > aer_recover_work_func # another thread > > kfifo_get(&aer_recover_ring) # remove from queue > > pci_print_aer > > __aer_print_error <--- > > > > aer_irq # native AER > > kfifo_put(&aer_fifo) # add to queue > > aer_isr # another thread > > kfifo_get(&aer_fifo) # remove from queue > > ... > > aer_isr_one_error > > aer_process_err_devices > > aer_print_error > > __aer_print_error <--- > > > > extlog_print # extlog (x86 only) > > apei_estatus_for_each_section > > ... > > if (guid_equal(sec_type, &CPER_SEC_PCIE)) > > extlog_print_pcie > > cper_severity_to_aer > > pci_get_domain_bus_and_slot > > pci_print_aer > > __aer_print_error <--- > > > > And we also have CXL paths that lead to __aer_print_error(), although > > it seems like they at least start in the native AER (and maybe GHES?) > > path and branch out somewhere. My head is spinning. > > > > Do I *object* to this patch? No, not really; it's a trivial change in > > drivers/pci/, and Rafael can add my > > > > Acked-by: Bjorn Helgaas <bhelgaas@google.com> > > > > as needed. But I am afraid we're making ourselves a maintenance > > headache. > > To be honest, I am too. Upon discovering that extlog_print() behaves > differently than ghes_do_proc(), I had the snarky thought "great, can we > now just go ahead and deprecate the extlog path, it's just a source of > maintenance pain.". > > So *if*we keep acpi_extlog it then I definitely think it should be > consistent with other CPER handlers (needs this patch). But, I am also > open to entertaining "deprecate it". > > To me, the fact that ras_userspace_consumers() is only honored for > acpi_extlog is clear evidence that the kernel has already painted itself > into a confusing user ABI corner and maybe the proper path forward at > this point is to cut acpi_extlog loose.
On Tuesday, August 6, 2024 9:56:24 PM GMT+2 Dan Williams wrote: > Fabio M. De Francesco wrote: > > Currently, extlog_print() (ELOG) only reports CPER PCIe section (UEFI > > v2.10, Appendix N.2.7) to the kernel log via print_extlog_rcd(). > > I think the critical detail is is that print_extlog_rcd() is only > triggered when ras_userspace_consumers() returns true. The observation > is that ras_userspace_consumers() hides information from the trace path > when the intended purpose of it was to hide duplicate emissions to the > kernel log when userspace is watching the tracepoints. > > Setting aside whether ras_userspace_consumers() is still a good idea or > not, it is obvious that this patch as is may surprise environments that > start seeing kernel error logs where the kernel was silent before. > > I think the path of least surprise would be to make sure that > pci_print_aer() optionally skips emitting to the kernel log when not > needed wanted. Sorry for replying so late... I'm not entirely sure that users would not prefer to be surprised by _finally_ seeing kernel error logs for failing PCIe components. I suspect that users might have been confused by not seeing any output. > So perhaps first do a lead-in patch to optionally quiet the print > messages in pci_print_aer() and then pass in KERN_DEBUG from the > extlog_print() path. Then we can decide later what to do about > ras_userspace_consumers(). Anyway, I'll do it. > > the similar ghes_do_proc() (GHES) prints to kernel log and calls > > pci_print_aer() to report via the ftrace infrastructure. > > > > Add support to report the CPER PCIe Error section also via the ftrace > > infrastructure by calling pci_print_aer() to make ELOG act consistently > > with GHES. > > You might also want to explain a bit about the motivation for this which > is that I/O Machine Check Arcitecture events may signal failing PCIe > components or links. The AER event contains details on what was > happening on the wire when the error was signaled. Yes, I agree. > > > > Cc: Dan Williams <dan.j.williams@intel.com> > > Signed-off-by: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com> > > --- > > drivers/acpi/acpi_extlog.c | 30 ++++++++++++++++++++++++++++++ > > drivers/pci/pcie/aer.c | 2 +- > > include/linux/aer.h | 13 +++++++++++-- > > 3 files changed, 42 insertions(+), 3 deletions(-) > > > > [...] > > > > + pci_print_aer(pdev, aer_severity, aer); > > ...per above this would become: > > pci_print_aer(KERN_DEBUG, pdev, aer_severity, aer); > > [..] > > Rest of the changes look good to me. I need to be sure that I understood... void pci_print_aer(char *level, struct pci_dev *dev, int aer_severity, struct aer_capability_regs *aer) { [...] if (printk_get_level(level) <= console_loglevel) { pci_err(dev, "aer_status: 0x%08x, aer_mask: 0x%08x\n", status, mask); __aer_print_error(dev, &info); pci_err(dev, "aer_layer=%s, aer_agent=%s\n", aer_error_layer[layer], aer_agent_string[agent]); if (aer_severity != AER_CORRECTABLE) pci_err(dev, "aer_uncor_severity: 0x%08x\n", aer->uncor_severity); if (tlp_header_valid) __print_tlp_header(dev, &aer->header_log); } [...] } It would require changing a couple of call sites, like in aer_recover_work_func(): pci_print_aer(KERN_ERR, pdev, entry.severity, entry.regs); Would you please confirm that the code shown above is what you asked for? Thanks, Fabio
diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c index e025ae390737..007ce96f8672 100644 --- a/drivers/acpi/acpi_extlog.c +++ b/drivers/acpi/acpi_extlog.c @@ -131,6 +131,32 @@ static int print_extlog_rcd(const char *pfx, return 1; } +static void extlog_print_pcie(struct cper_sec_pcie *pcie_err, + int severity) +{ + struct aer_capability_regs *aer; + struct pci_dev *pdev; + unsigned int devfn; + unsigned int bus; + int aer_severity; + int domain; + + if (pcie_err->validation_bits & CPER_PCIE_VALID_DEVICE_ID && + pcie_err->validation_bits & CPER_PCIE_VALID_AER_INFO) { + aer_severity = cper_severity_to_aer(severity); + aer = (struct aer_capability_regs *)pcie_err->aer_info; + domain = pcie_err->device_id.segment; + bus = pcie_err->device_id.bus; + devfn = PCI_DEVFN(pcie_err->device_id.device, + pcie_err->device_id.function); + pdev = pci_get_domain_bus_and_slot(domain, bus, devfn); + if (!pdev) + return; + pci_print_aer(pdev, aer_severity, aer); + pci_dev_put(pdev); + } +} + static int extlog_print(struct notifier_block *nb, unsigned long val, void *data) { @@ -179,6 +205,10 @@ static int extlog_print(struct notifier_block *nb, unsigned long val, if (gdata->error_data_length >= sizeof(*mem)) trace_extlog_mem_event(mem, err_seq, fru_id, fru_text, (u8)gdata->error_severity); + } else if (guid_equal(sec_type, &CPER_SEC_PCIE)) { + struct cper_sec_pcie *pcie_err = acpi_hest_get_payload(gdata); + + extlog_print_pcie(pcie_err, gdata->error_severity); } else { void *err = acpi_hest_get_payload(gdata); diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c index ac6293c24976..794aa15527ba 100644 --- a/drivers/pci/pcie/aer.c +++ b/drivers/pci/pcie/aer.c @@ -801,7 +801,7 @@ void pci_print_aer(struct pci_dev *dev, int aer_severity, trace_aer_event(dev_name(&dev->dev), (status & ~mask), aer_severity, tlp_header_valid, &aer->header_log); } -EXPORT_SYMBOL_NS_GPL(pci_print_aer, CXL); +EXPORT_SYMBOL_GPL(pci_print_aer); /** * add_error_device - list device to be handled diff --git a/include/linux/aer.h b/include/linux/aer.h index 4b97f38f3fcf..fbc82206045c 100644 --- a/include/linux/aer.h +++ b/include/linux/aer.h @@ -42,17 +42,26 @@ int pcie_read_tlp_log(struct pci_dev *dev, int where, struct pcie_tlp_log *log); #if defined(CONFIG_PCIEAER) int pci_aer_clear_nonfatal_status(struct pci_dev *dev); int pcie_aer_is_native(struct pci_dev *dev); +void pci_print_aer(struct pci_dev *dev, int aer_severity, + struct aer_capability_regs *aer); #else static inline int pci_aer_clear_nonfatal_status(struct pci_dev *dev) { return -EINVAL; } + static inline int pcie_aer_is_native(struct pci_dev *dev) { return 0; } +static inline void pci_print_aer(struct pci_dev *dev, int aer_severity, + struct aer_capability_regs *aer) +{ } #endif -void pci_print_aer(struct pci_dev *dev, int aer_severity, - struct aer_capability_regs *aer); +#if defined(CONFIG_ACPI_APEI_PCIEAER) int cper_severity_to_aer(int cper_severity); +#else +static inline int cper_severity_to_aer(int cper_severity) { return 0; } +#endif + void aer_recover_queue(int domain, unsigned int bus, unsigned int devfn, int severity, struct aer_capability_regs *aer_regs); #endif //_AER_H_
Currently, extlog_print() (ELOG) only reports CPER PCIe section (UEFI v2.10, Appendix N.2.7) to the kernel log via print_extlog_rcd(). Instead, the similar ghes_do_proc() (GHES) prints to kernel log and calls pci_print_aer() to report via the ftrace infrastructure. Add support to report the CPER PCIe Error section also via the ftrace infrastructure by calling pci_print_aer() to make ELOG act consistently with GHES. Cc: Dan Williams <dan.j.williams@intel.com> Signed-off-by: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com> --- drivers/acpi/acpi_extlog.c | 30 ++++++++++++++++++++++++++++++ drivers/pci/pcie/aer.c | 2 +- include/linux/aer.h | 13 +++++++++++-- 3 files changed, 42 insertions(+), 3 deletions(-)