[PATCH 0/4] efi/cxl-cper: Report CXL CPER events through tracing

[PATCH 0/4] efi/cxl-cper: Report CXL CPER events through tracing

[Ira Weiny]

CXL Component Events, as defined by EFI 2.10 Section N.2.14, wrap a
mostly CXL event payload in an EFI Common Platform Error Record (CPER)
record.  If a device is configured for firmware first CXL event records
are not sent directly to the host.

The CXL sub-system uniquely has DPA to HPA translation information.  It
also already has event decoding/tracing.  Such translations are very
useful for users to determine which system issues may correspond to
specific hardware events.

The restructuring of the event data structures in 6.8 made sharing the
data between CPER/event logs more efficient.  Now re-wire the sending of
CPER records to the CXL sub-system.

In addition provide a default RAS event should the CXL module not be
loaded [ie callback not registered].

Series status/background
========================

Smita and Jonathan have been a great help with this series.  Once again
thank you.

Unfortunately, with all the churn surrounding the bug which Dan
Carpenter found the maintainers were force to revert this work.

Therefore, this is a whole new series based on what is in 6.8.

Testing
=======

I've hacked up a quick debugfs patch to facilitate easier testing.[1]

With this I have verified that the bug Dan Carpenter found is fixed.
However, the tp_printk bug Jonathan found remains.  The taking of the
device lock in the callback is required and the tp_printk issue is
unlikely to be fixed.  Fortunately, tp_printk is not widely used so it
is anticipated this will not be an issue.

No other locking issues were found with this test and locking debug
turned on.

[1] https://github.com/weiny2/linux-kernel/commit/6c540a23cb1194d67a9dcfefb702774a99afc3b1

Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
Ira Weiny (4):
      cxl/event: Add missing include files
      acpi/ghes: Process CXL Component Events
      cxl/pci: Register for and process CPER events
      ras/events: Trace CXL CPER events even without the CXL stack loaded

 drivers/acpi/apei/ghes.c  | 130 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/cxl/pci.c         |  69 +++++++++++++++++++++++-
 include/linux/cxl-event.h |  21 ++++++++
 include/ras/ras_event.h   |  90 ++++++++++++++++++++++++++++++++
 4 files changed, 309 insertions(+), 1 deletion(-)
---
base-commit: daeacfa75d08954e1a5b71c36a8fbfcdd0b3fec9
change-id: 20240220-cxl-cper3-30e55279f936

Best regards,
-- 
Ira Weiny <ira.weiny@intel.com>

[PATCH 1/4] cxl/event: Add missing include files

[Ira Weiny]

Additional event testing using the cxl-event.h header revealed that it
did not include the correct headers for the types used.  Compile errors
such as:

	include/linux/cxl-event.h|11 col 9| error: unknown type name ‘u8’

... were seen.

Add the correct pre-requisite headers.

Omit the fixes tag because this does not cause any issues until the
header is used again in other code.

Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 include/linux/cxl-event.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/cxl-event.h b/include/linux/cxl-event.h
index 03fa6d50d46f..812ed16ffc2f 100644
--- a/include/linux/cxl-event.h
+++ b/include/linux/cxl-event.h
@@ -3,6 +3,9 @@
 #ifndef _LINUX_CXL_EVENT_H
 #define _LINUX_CXL_EVENT_H
 
+#include <linux/types.h>
+#include <linux/uuid.h>
+
 /*
  * Common Event Record Format
  * CXL rev 3.0 section 8.2.9.2.1; Table 8-42

-- 
2.43.0

Re: [PATCH 1/4] cxl/event: Add missing include files

[Dan Williams]

Ira Weiny wrote:
> Additional event testing using the cxl-event.h header revealed that it
> did not include the correct headers for the types used.  Compile errors
> such as:
> 
> 	include/linux/cxl-event.h|11 col 9| error: unknown type name ‘u8’
> 
> ... were seen.

Were seen where? Should this have the trio of Reported-by: Closes: and
Fixes tags?

Re: [PATCH 1/4] cxl/event: Add missing include files

[Ira Weiny]

Dan Williams wrote:
> Ira Weiny wrote:
> > Additional event testing using the cxl-event.h header revealed that it
> > did not include the correct headers for the types used.  Compile errors
> > such as:
> > 
> > 	include/linux/cxl-event.h|11 col 9| error: unknown type name ‘u8’
> > 
> > ... were seen.
> 
> Were seen where? Should this have the trio of Reported-by: Closes: and
> Fixes tags?

As I said after this in the commit message:

	"Omit the fixes tag because this does not cause any issues until
	the header is used again in other code."

I'll clarify it was seen when I used cxl-event.h in the testing code and
this happened.

Ira

[PATCH 2/4] acpi/ghes: Process CXL Component Events

[Ira Weiny]

BIOS can configure memory devices as firmware first.  This will send CXL
events to the firmware instead of the OS.  The firmware can then send
these events to the OS via UEFI.  Currently a configuration such as this
will trace a non standard event in the log.  Using the specific CXL
trace points with the additional information CXL can provide is much
more useful to users.  Specifically, future support can be added to CXL
provide the DPA to HPA mapping configured at the time of the event.

UEFI v2.10 section N.2.14 defines a Common Platform Error Record (CPER)
format for CXL Component Events.  The format is mostly the same as the
CXL Common Event Record Format.  The difference is the use of a GUID in
the Section Type rather than a UUID as part of the event itself.

Add GHES support to detect CXL CPER records and call into the CXL code
to process the event.

Multiple methods were considered for the call into the CXL code.  A
notifier chain was considered for the callback but the complexity did
not justify the use case.  Furthermore, the CXL code is required to be
called from process context as it needs to take a device lock so a
simple callback register proved difficult.  Dan Williams suggested
using 2 work items as an atomic way of switching between a callback
being registered and not.  This allows the callback to run without any
locking.[1]

Note that a local work item is required to dump any messages seen during
a race between any check done in cxl_cper_post_event() and the
scheduling of work.  That said, no attempt is made to stop the addition
of messages into the kfifo because this local work item provides a hook
to add a local CXL CPER trace point in a future patch.

This new combined patch addresses the report by Dan Carpenter[2].  Thus
the reported by tag.

[1] https://lore.kernel.org/all/65d111eb87115_6c745294ac@dwillia2-xfh.jf.intel.com.notmuch/
[2] https://lore.kernel.org/all/b963c490-2c13-4b79-bbe7-34c6568423c7@moroto.mountain/

Cc: Ard Biesheuvel <ardb@kernel.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Borislav Petkov <bp@alien8.de>
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Suggested-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Ira Weiny <ira.weiny@intel.com>

---
[djbw: use kfifo for record data]
[djbw: Use work struct for sync between cxl reg and ghes code]
---
 drivers/acpi/apei/ghes.c  | 127 ++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/cxl-event.h |  18 +++++++
 2 files changed, 145 insertions(+)

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index ab2a82cb1b0b..f433f4eae888 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -26,6 +26,7 @@
 #include <linux/interrupt.h>
 #include <linux/timer.h>
 #include <linux/cper.h>
+#include <linux/cxl-event.h>
 #include <linux/platform_device.h>
 #include <linux/mutex.h>
 #include <linux/ratelimit.h>
@@ -33,6 +34,7 @@
 #include <linux/irq_work.h>
 #include <linux/llist.h>
 #include <linux/genalloc.h>
+#include <linux/kfifo.h>
 #include <linux/pci.h>
 #include <linux/pfn.h>
 #include <linux/aer.h>
@@ -673,6 +675,116 @@ static void ghes_defer_non_standard_event(struct acpi_hest_generic_data *gdata,
 	schedule_work(&entry->work);
 }
 
+/* CXL Event record UUIDs are formated as GUIDs and reported in section type */
+
+/*
+ * General Media Event Record
+ * CXL rev 3.0 Section 8.2.9.2.1.1; Table 8-43
+ */
+#define CPER_SEC_CXL_GEN_MEDIA_GUID					\
+	GUID_INIT(0xfbcd0a77, 0xc260, 0x417f,				\
+		  0x85, 0xa9, 0x08, 0x8b, 0x16, 0x21, 0xeb, 0xa6)
+
+/*
+ * DRAM Event Record
+ * CXL rev 3.0 section 8.2.9.2.1.2; Table 8-44
+ */
+#define CPER_SEC_CXL_DRAM_GUID						\
+	GUID_INIT(0x601dcbb3, 0x9c06, 0x4eab,				\
+		  0xb8, 0xaf, 0x4e, 0x9b, 0xfb, 0x5c, 0x96, 0x24)
+
+/*
+ * Memory Module Event Record
+ * CXL rev 3.0 section 8.2.9.2.1.3; Table 8-45
+ */
+#define CPER_SEC_CXL_MEM_MODULE_GUID					\
+	GUID_INIT(0xfe927475, 0xdd59, 0x4339,				\
+		  0xa5, 0x86, 0x79, 0xba, 0xb1, 0x13, 0xb7, 0x74)
+
+struct cxl_cper_work_data {
+	enum cxl_event_type event_type;
+	struct cxl_cper_event_rec rec;
+};
+
+DEFINE_KFIFO(cxl_cper_fifo, struct cxl_cper_work_data, 32);
+static DEFINE_SPINLOCK(cxl_cper_read_lock);
+static DEFINE_SPINLOCK(cxl_cper_write_lock);
+
+static cxl_cper_callback cper_callback;
+/* cb function dumps the records */
+static void cxl_cper_cb_fn(struct work_struct *work)
+{
+	struct cxl_cper_work_data wd;
+
+	while (kfifo_out_spinlocked(&cxl_cper_fifo, &wd, 1,
+			&cxl_cper_read_lock)) {
+		cper_callback(wd.event_type, &wd.rec);
+	}
+}
+static DECLARE_WORK(cxl_cb_work, cxl_cper_cb_fn);
+
+static void cxl_cper_local_fn(struct work_struct *work)
+{
+	struct cxl_cper_work_data wd;
+
+	while (kfifo_out_spinlocked(&cxl_cper_fifo, &wd, 1,
+			&cxl_cper_read_lock)) {
+		/* drop msg */
+	}
+}
+static DECLARE_WORK(cxl_local_work, cxl_cper_local_fn);
+
+/* Pointer for atomic switch of record processing */
+struct work_struct *cxl_cper_work = &cxl_local_work;
+
+static void cxl_cper_post_event(enum cxl_event_type event_type,
+				struct cxl_cper_event_rec *rec)
+{
+	struct cxl_cper_work_data wd;
+
+	if (rec->hdr.length <= sizeof(rec->hdr) ||
+	    rec->hdr.length > sizeof(*rec)) {
+		pr_err(FW_WARN "CXL CPER Invalid section length (%u)\n",
+		       rec->hdr.length);
+		return;
+	}
+
+	if (!(rec->hdr.validation_bits & CPER_CXL_COMP_EVENT_LOG_VALID)) {
+		pr_err(FW_WARN "CXL CPER invalid event\n");
+		return;
+	}
+
+	wd.event_type = event_type;
+	memcpy(&wd.rec, rec, sizeof(wd.rec));
+
+	kfifo_in_spinlocked(&cxl_cper_fifo, &wd, 1, &cxl_cper_write_lock);
+	schedule_work(cxl_cper_work);
+}
+
+int cxl_cper_register_callback(cxl_cper_callback callback)
+{
+	if (cper_callback)
+		return -EINVAL;
+	cper_callback = callback;
+	/* Atomic switch back to callback processing */
+	cxl_cper_work = &cxl_cb_work;
+	return 0;
+}
+EXPORT_SYMBOL_NS_GPL(cxl_cper_register_callback, CXL);
+
+int cxl_cper_unregister_callback(cxl_cper_callback callback)
+{
+	if (callback != cper_callback)
+		return -EINVAL;
+
+	/* Atomic switch back to ghes processing */
+	cxl_cper_work = &cxl_local_work;
+	cancel_work_sync(&cxl_cb_work);
+	cper_callback = NULL;
+	return 0;
+}
+EXPORT_SYMBOL_NS_GPL(cxl_cper_unregister_callback, CXL);
+
 static bool ghes_do_proc(struct ghes *ghes,
 			 const struct acpi_hest_generic_status *estatus)
 {
@@ -707,6 +819,21 @@ static bool ghes_do_proc(struct ghes *ghes,
 		}
 		else if (guid_equal(sec_type, &CPER_SEC_PROC_ARM)) {
 			queued = ghes_handle_arm_hw_error(gdata, sev, sync);
+		}
+		else if (guid_equal(sec_type, &CPER_SEC_CXL_GEN_MEDIA_GUID)) {
+			struct cxl_cper_event_rec *rec = acpi_hest_get_payload(gdata);
+
+			cxl_cper_post_event(CXL_CPER_EVENT_GEN_MEDIA, rec);
+		}
+		else if (guid_equal(sec_type, &CPER_SEC_CXL_DRAM_GUID)) {
+			struct cxl_cper_event_rec *rec = acpi_hest_get_payload(gdata);
+
+			cxl_cper_post_event(CXL_CPER_EVENT_DRAM, rec);
+		}
+		else if (guid_equal(sec_type, &CPER_SEC_CXL_MEM_MODULE_GUID)) {
+			struct cxl_cper_event_rec *rec = acpi_hest_get_payload(gdata);
+
+			cxl_cper_post_event(CXL_CPER_EVENT_MEM_MODULE, rec);
 		} else {
 			void *err = acpi_hest_get_payload(gdata);
 
diff --git a/include/linux/cxl-event.h b/include/linux/cxl-event.h
index 812ed16ffc2f..4834cf23656e 100644
--- a/include/linux/cxl-event.h
+++ b/include/linux/cxl-event.h
@@ -143,4 +143,22 @@ struct cxl_cper_event_rec {
 	union cxl_event event;
 } __packed;
 
+typedef void (*cxl_cper_callback)(enum cxl_event_type type,
+				  struct cxl_cper_event_rec *rec);
+
+#ifdef CONFIG_ACPI_APEI_GHES
+int cxl_cper_register_callback(cxl_cper_callback callback);
+int cxl_cper_unregister_callback(cxl_cper_callback callback);
+#else
+static inline int cxl_cper_register_callback(cxl_cper_callback callback)
+{
+	return 0;
+}
+
+static inline int cxl_cper_unregister_callback(cxl_cper_callback callback)
+{
+	return 0;
+}
+#endif
+
 #endif /* _LINUX_CXL_EVENT_H */

-- 
2.43.0

Re: [PATCH 2/4] acpi/ghes: Process CXL Component Events

[Dan Williams]

Ira Weiny wrote:
> BIOS can configure memory devices as firmware first.  This will send CXL
> events to the firmware instead of the OS.  The firmware can then send
> these events to the OS via UEFI.  Currently a configuration such as this
> will trace a non standard event in the log.  Using the specific CXL
> trace points with the additional information CXL can provide is much
> more useful to users.  Specifically, future support can be added to CXL
> provide the DPA to HPA mapping configured at the time of the event.

One could argue that support should have happened first and taken the
event all the way to EDAC, so this needs to merged on faith that that
those patches are in flight.

> UEFI v2.10 section N.2.14 defines a Common Platform Error Record (CPER)
> format for CXL Component Events.  The format is mostly the same as the
> CXL Common Event Record Format.  The difference is the use of a GUID in
> the Section Type rather than a UUID as part of the event itself.
> 
> Add GHES support to detect CXL CPER records and call into the CXL code
> to process the event.
> 
> Multiple methods were considered for the call into the CXL code.  A
> notifier chain was considered for the callback but the complexity did
> not justify the use case.

Not sure what this adds. If you want to talk about alternatives and
tradeoffs, great, but that should be a comparative analysis in support
of the chosen direction.

> Furthermore, the CXL code is required to be called from process
> context as it needs to take a device lock so a simple callback
> register proved difficult.  Dan Williams suggested using 2 work items
> as an atomic way of switching between a callback being registered and
> not.  This allows the callback to run without any locking.[1]
> 
> Note that a local work item is required to dump any messages seen during
> a race between any check done in cxl_cper_post_event() and the
> scheduling of work.  That said, no attempt is made to stop the addition
> of messages into the kfifo because this local work item provides a hook
> to add a local CXL CPER trace point in a future patch.
> 
> This new combined patch addresses the report by Dan Carpenter[2].  Thus
> the reported by tag.
> 
> [1] https://lore.kernel.org/all/65d111eb87115_6c745294ac@dwillia2-xfh.jf.intel.com.notmuch/
> [2] https://lore.kernel.org/all/b963c490-2c13-4b79-bbe7-34c6568423c7@moroto.mountain/
> 
> Cc: Ard Biesheuvel <ardb@kernel.org>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: Tony Luck <tony.luck@intel.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>

checkpatch will whine about a missing Closes: tag.

> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> 
> ---
> [djbw: use kfifo for record data]
> [djbw: Use work struct for sync between cxl reg and ghes code]
> ---
>  drivers/acpi/apei/ghes.c  | 127 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/cxl-event.h |  18 +++++++
>  2 files changed, 145 insertions(+)
> 
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index ab2a82cb1b0b..f433f4eae888 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -26,6 +26,7 @@
>  #include <linux/interrupt.h>
>  #include <linux/timer.h>
>  #include <linux/cper.h>
> +#include <linux/cxl-event.h>
>  #include <linux/platform_device.h>
>  #include <linux/mutex.h>
>  #include <linux/ratelimit.h>
> @@ -33,6 +34,7 @@
>  #include <linux/irq_work.h>
>  #include <linux/llist.h>
>  #include <linux/genalloc.h>
> +#include <linux/kfifo.h>
>  #include <linux/pci.h>
>  #include <linux/pfn.h>
>  #include <linux/aer.h>
> @@ -673,6 +675,116 @@ static void ghes_defer_non_standard_event(struct acpi_hest_generic_data *gdata,
>  	schedule_work(&entry->work);
>  }
>  
> +/* CXL Event record UUIDs are formated as GUIDs and reported in section type */
> +
> +/*
> + * General Media Event Record
> + * CXL rev 3.0 Section 8.2.9.2.1.1; Table 8-43
> + */
> +#define CPER_SEC_CXL_GEN_MEDIA_GUID					\
> +	GUID_INIT(0xfbcd0a77, 0xc260, 0x417f,				\
> +		  0x85, 0xa9, 0x08, 0x8b, 0x16, 0x21, 0xeb, 0xa6)
> +
> +/*
> + * DRAM Event Record
> + * CXL rev 3.0 section 8.2.9.2.1.2; Table 8-44
> + */
> +#define CPER_SEC_CXL_DRAM_GUID						\
> +	GUID_INIT(0x601dcbb3, 0x9c06, 0x4eab,				\
> +		  0xb8, 0xaf, 0x4e, 0x9b, 0xfb, 0x5c, 0x96, 0x24)
> +
> +/*
> + * Memory Module Event Record
> + * CXL rev 3.0 section 8.2.9.2.1.3; Table 8-45
> + */
> +#define CPER_SEC_CXL_MEM_MODULE_GUID					\
> +	GUID_INIT(0xfe927475, 0xdd59, 0x4339,				\
> +		  0xa5, 0x86, 0x79, 0xba, 0xb1, 0x13, 0xb7, 0x74)
> +
> +struct cxl_cper_work_data {
> +	enum cxl_event_type event_type;
> +	struct cxl_cper_event_rec rec;
> +};
> +
> +DEFINE_KFIFO(cxl_cper_fifo, struct cxl_cper_work_data, 32);
> +static DEFINE_SPINLOCK(cxl_cper_read_lock);
> +static DEFINE_SPINLOCK(cxl_cper_write_lock);
> +
> +static cxl_cper_callback cper_callback;
> +/* cb function dumps the records */
> +static void cxl_cper_cb_fn(struct work_struct *work)
> +{
> +	struct cxl_cper_work_data wd;
> +
> +	while (kfifo_out_spinlocked(&cxl_cper_fifo, &wd, 1,
> +			&cxl_cper_read_lock)) {

Why is this taking the lock on retrieval? The work item is
single-threaded. The only potential contention is between
cxl_cper_local_fn() and cxl_cper_cb_fn() collision, but that can be
handled by a cancel_work_sync(&cxl_local_work) on registration to pair
with the cancel_work_sync(&cxl_cb_work) on unregistration.

...but I am not sure 2 work items are needed unless some default
processing is going to happen in the "local" case.

> +		cper_callback(wd.event_type, &wd.rec);
> +	}
> +}
> +static DECLARE_WORK(cxl_cb_work, cxl_cper_cb_fn);
> +
> +static void cxl_cper_local_fn(struct work_struct *work)
> +{
> +	struct cxl_cper_work_data wd;
> +
> +	while (kfifo_out_spinlocked(&cxl_cper_fifo, &wd, 1,
> +			&cxl_cper_read_lock)) {

This just looks like open coded / less efficient kfifo_reset_out().

> +		/* drop msg */

If the proposal is to do nothing when no callback is registered then no
need to have have cxl_local_work at all.

> +	}
> +}
> +static DECLARE_WORK(cxl_local_work, cxl_cper_local_fn);
> +
> +/* Pointer for atomic switch of record processing */
> +struct work_struct *cxl_cper_work = &cxl_local_work;
> +
> +static void cxl_cper_post_event(enum cxl_event_type event_type,
> +				struct cxl_cper_event_rec *rec)
> +{
> +	struct cxl_cper_work_data wd;
> +
> +	if (rec->hdr.length <= sizeof(rec->hdr) ||
> +	    rec->hdr.length > sizeof(*rec)) {
> +		pr_err(FW_WARN "CXL CPER Invalid section length (%u)\n",
> +		       rec->hdr.length);
> +		return;
> +	}
> +
> +	if (!(rec->hdr.validation_bits & CPER_CXL_COMP_EVENT_LOG_VALID)) {
> +		pr_err(FW_WARN "CXL CPER invalid event\n");
> +		return;
> +	}
> +
> +	wd.event_type = event_type;
> +	memcpy(&wd.rec, rec, sizeof(wd.rec));

Unfortunate to have a double copy of the record into the stack variable
and then again into the kfifo, but I can not immediately think of a way
around that.

> +
> +	kfifo_in_spinlocked(&cxl_cper_fifo, &wd, 1, &cxl_cper_write_lock);
> +	schedule_work(cxl_cper_work);

I think you don't need 2 work items if you write it this way:

        spin_lock_irqsave(&cxl_cper_write_lock, flags);
        if (cxl_cper_work) {
                if (kfifo_put(&cxl_cper_fifo, &wd))
                        schedule_work(cxl_cper_work);
                else
                        pr_err_ratelimited(
                                "buffer overflow when queuing CXL CPER record\n");
        }
        spin_lock_irqrestore(&cxl_cper_write_lock, flags);

...and then take the write lock when modifying that cxl_cper_work
pointer between NULL and non-NULL.

Re: [PATCH 2/4] acpi/ghes: Process CXL Component Events

[Ira Weiny]

Dan Williams wrote:
> Ira Weiny wrote:
> > BIOS can configure memory devices as firmware first.  This will send CXL
> > events to the firmware instead of the OS.  The firmware can then send
> > these events to the OS via UEFI.  Currently a configuration such as this
> > will trace a non standard event in the log.  Using the specific CXL
> > trace points with the additional information CXL can provide is much
                         ^^^^^^^^^^^^^^^^^^^^^^^^
			 See below.
> > more useful to users.
> > Specifically, future support can be added to CXL
> > provide the DPA to HPA mapping configured at the time of the event.
> 
> One could argue that support should have happened first and taken the
> event all the way to EDAC, so this needs to merged on faith that that
> those patches are in flight.

Sure but then you also don't get the additional information already
provided by the CXL trace points.  Reading this back I see that my use of
'Specifically' masked the fact that the CXL code already has better
support to decode these records and we want to leverage that.

> 
> > UEFI v2.10 section N.2.14 defines a Common Platform Error Record (CPER)
> > format for CXL Component Events.  The format is mostly the same as the
> > CXL Common Event Record Format.  The difference is the use of a GUID in
> > the Section Type rather than a UUID as part of the event itself.
> > 
> > Add GHES support to detect CXL CPER records and call into the CXL code
> > to process the event.
> > 
> > Multiple methods were considered for the call into the CXL code.  A
> > notifier chain was considered for the callback but the complexity did
> > not justify the use case.
> 
> Not sure what this adds. If you want to talk about alternatives and
> tradeoffs, great, but that should be a comparative analysis in support
> of the chosen direction.

I'll remove it.  This was carried from the previous versions.

> 
> > Furthermore, the CXL code is required to be called from process
> > context as it needs to take a device lock so a simple callback
> > register proved difficult.  Dan Williams suggested using 2 work items
> > as an atomic way of switching between a callback being registered and
> > not.  This allows the callback to run without any locking.[1]
> > 
> > Note that a local work item is required to dump any messages seen during
> > a race between any check done in cxl_cper_post_event() and the
> > scheduling of work.  That said, no attempt is made to stop the addition
> > of messages into the kfifo because this local work item provides a hook
> > to add a local CXL CPER trace point in a future patch.
> > 
> > This new combined patch addresses the report by Dan Carpenter[2].  Thus
> > the reported by tag.
> > 
> > [1] https://lore.kernel.org/all/65d111eb87115_6c745294ac@dwillia2-xfh.jf.intel.com.notmuch/
> > [2] https://lore.kernel.org/all/b963c490-2c13-4b79-bbe7-34c6568423c7@moroto.mountain/
> > 
> > Cc: Ard Biesheuvel <ardb@kernel.org>
> > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > Cc: Tony Luck <tony.luck@intel.com>
> > Cc: Borislav Petkov <bp@alien8.de>
> > Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> 
> checkpatch will whine about a missing Closes: tag.

I know.  How about Suggested-by?  I want to give Dan credit because your
revert already closed the actual bug.


[snip]

> > +
> > +DEFINE_KFIFO(cxl_cper_fifo, struct cxl_cper_work_data, 32);
> > +static DEFINE_SPINLOCK(cxl_cper_read_lock);
> > +static DEFINE_SPINLOCK(cxl_cper_write_lock);
> > +
> > +static cxl_cper_callback cper_callback;
> > +/* cb function dumps the records */
> > +static void cxl_cper_cb_fn(struct work_struct *work)
> > +{
> > +	struct cxl_cper_work_data wd;
> > +
> > +	while (kfifo_out_spinlocked(&cxl_cper_fifo, &wd, 1,
> > +			&cxl_cper_read_lock)) {
> 
> Why is this taking the lock on retrieval? The work item is
> single-threaded. The only potential contention is between
> cxl_cper_local_fn() and cxl_cper_cb_fn() collision, but that can be
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  Because of this collision, Yes.

> handled by a cancel_work_sync(&cxl_local_work) on registration to pair
> with the cancel_work_sync(&cxl_cb_work) on unregistration.

Ok yea I could use cancel_work_sync().

> 
> ...but I am not sure 2 work items are needed unless some default
> processing is going to happen in the "local" case.

I thought about merging patch 3 in this one and that would make the use of
this work function more clear.

> 
> > +		cper_callback(wd.event_type, &wd.rec);
> > +	}
> > +}
> > +static DECLARE_WORK(cxl_cb_work, cxl_cper_cb_fn);
> > +
> > +static void cxl_cper_local_fn(struct work_struct *work)
> > +{
> > +	struct cxl_cper_work_data wd;
> > +
> > +	while (kfifo_out_spinlocked(&cxl_cper_fifo, &wd, 1,
> > +			&cxl_cper_read_lock)) {
> 
> This just looks like open coded / less efficient kfifo_reset_out().
> 
> > +		/* drop msg */
> 
> If the proposal is to do nothing when no callback is registered then no
> need to have have cxl_local_work at all.

Patch 4 makes use of this.

> 
> > +	}
> > +}
> > +static DECLARE_WORK(cxl_local_work, cxl_cper_local_fn);
> > +
> > +/* Pointer for atomic switch of record processing */
> > +struct work_struct *cxl_cper_work = &cxl_local_work;
> > +
> > +static void cxl_cper_post_event(enum cxl_event_type event_type,
> > +				struct cxl_cper_event_rec *rec)
> > +{
> > +	struct cxl_cper_work_data wd;
> > +
> > +	if (rec->hdr.length <= sizeof(rec->hdr) ||
> > +	    rec->hdr.length > sizeof(*rec)) {
> > +		pr_err(FW_WARN "CXL CPER Invalid section length (%u)\n",
> > +		       rec->hdr.length);
> > +		return;
> > +	}
> > +
> > +	if (!(rec->hdr.validation_bits & CPER_CXL_COMP_EVENT_LOG_VALID)) {
> > +		pr_err(FW_WARN "CXL CPER invalid event\n");
> > +		return;
> > +	}
> > +
> > +	wd.event_type = event_type;
> > +	memcpy(&wd.rec, rec, sizeof(wd.rec));
> 
> Unfortunate to have a double copy of the record into the stack variable
> and then again into the kfifo, but I can not immediately think of a way
> around that.

Yep.  I could not either.

> 
> > +
> > +	kfifo_in_spinlocked(&cxl_cper_fifo, &wd, 1, &cxl_cper_write_lock);
> > +	schedule_work(cxl_cper_work);
> 
> I think you don't need 2 work items if you write it this way:
> 
>         spin_lock_irqsave(&cxl_cper_write_lock, flags);
>         if (cxl_cper_work) {
>                 if (kfifo_put(&cxl_cper_fifo, &wd))
>                         schedule_work(cxl_cper_work);
>                 else
>                         pr_err_ratelimited(
>                                 "buffer overflow when queuing CXL CPER record\n");
>         }
>         spin_lock_irqrestore(&cxl_cper_write_lock, flags);
> 
> ...and then take the write lock when modifying that cxl_cper_work
> pointer between NULL and non-NULL.

I'll merge patch 4 here.  Then this will be used.

Ira

[PATCH 3/4] cxl/pci: Register for and process CPER events

[Ira Weiny]

If the firmware has configured CXL event support to be firmware first
the OS can process those events through CPER records.  The CXL layer has
unique DPA to HPA knowledge and standard event trace parsing in place.

CPER records contain Bus, Device, Function information which can be used
to identify the PCI device which is sending the event.

Add a CXL CPER callback to process events through the CXL trace
subsystem.

Future patches will provide additional region information such as HPA.

Signed-off-by: Ira Weiny <ira.weiny@intel.com>

---
Changes:
[iweiny: Add back in after the revert in 6.8]
---
 drivers/cxl/pci.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 68 insertions(+), 1 deletion(-)

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 2ff361e756d6..6cf8336d1b33 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -974,6 +974,73 @@ static struct pci_driver cxl_pci_driver = {
 	},
 };
 
-module_pci_driver(cxl_pci_driver);
+#define CXL_EVENT_HDR_FLAGS_REC_SEVERITY GENMASK(1, 0)
+static void cxl_cper_event_call(enum cxl_event_type ev_type,
+				struct cxl_cper_event_rec *rec)
+{
+	struct cper_cxl_event_devid *device_id = &rec->hdr.device_id;
+	struct pci_dev *pdev __free(pci_dev_put) = NULL;
+	enum cxl_event_log_type log_type;
+	struct cxl_dev_state *cxlds;
+	unsigned int devfn;
+	u32 hdr_flags;
+
+	pr_debug("CPER event for device %u:%u:%u.%u\n",
+		 device_id->segment_num, device_id->bus_num,
+		 device_id->device_num, device_id->func_num);
+
+	devfn = PCI_DEVFN(device_id->device_num, device_id->func_num);
+	pdev = pci_get_domain_bus_and_slot(device_id->segment_num,
+					   device_id->bus_num, devfn);
+	if (!pdev) {
+		pr_err("CPER event device %u:%u:%u.%u not found\n",
+			device_id->segment_num, device_id->bus_num,
+			device_id->device_num, device_id->func_num);
+		return;
+	}
+
+	dev_dbg(&pdev->dev, "Found device %u:%u.%u\n", device_id->bus_num,
+		device_id->device_num, device_id->func_num);
+
+	guard(device)(&pdev->dev);
+	if (pdev->driver != &cxl_pci_driver)
+		return;
+
+	cxlds = pci_get_drvdata(pdev);
+	if (!cxlds)
+		return;
+
+	/* Fabricate a log type */
+	hdr_flags = get_unaligned_le24(rec->event.generic.hdr.flags);
+	log_type = FIELD_GET(CXL_EVENT_HDR_FLAGS_REC_SEVERITY, hdr_flags);
+
+	dev_dbg(&pdev->dev, "Tracing %d\n", ev_type);
+	cxl_event_trace_record(cxlds->cxlmd, log_type, ev_type,
+			       &uuid_null, &rec->event);
+}
+
+static int __init cxl_pci_driver_init(void)
+{
+	int rc;
+
+	rc = pci_register_driver(&cxl_pci_driver);
+	if (rc)
+		return rc;
+
+	rc = cxl_cper_register_callback(cxl_cper_event_call);
+	if (rc)
+		pci_unregister_driver(&cxl_pci_driver);
+
+	return rc;
+}
+
+static void __exit cxl_pci_driver_exit(void)
+{
+	cxl_cper_unregister_callback(cxl_cper_event_call);
+	pci_unregister_driver(&cxl_pci_driver);
+}
+
+module_init(cxl_pci_driver_init);
+module_exit(cxl_pci_driver_exit);
 MODULE_LICENSE("GPL v2");
 MODULE_IMPORT_NS(CXL);

-- 
2.43.0

Re: [PATCH 3/4] cxl/pci: Register for and process CPER events

[Dan Williams]

Ira Weiny wrote:
> If the firmware has configured CXL event support to be firmware first
> the OS can process those events through CPER records.  The CXL layer has
> unique DPA to HPA knowledge and standard event trace parsing in place.
> 
> CPER records contain Bus, Device, Function information which can be used
> to identify the PCI device which is sending the event.
> 
> Add a CXL CPER callback to process events through the CXL trace
> subsystem.
> 
> Future patches will provide additional region information such as HPA.
> 
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> 
> ---
> Changes:
> [iweiny: Add back in after the revert in 6.8]
> ---
>  drivers/cxl/pci.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 68 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 2ff361e756d6..6cf8336d1b33 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -974,6 +974,73 @@ static struct pci_driver cxl_pci_driver = {
>  	},
>  };
>  
> -module_pci_driver(cxl_pci_driver);
> +#define CXL_EVENT_HDR_FLAGS_REC_SEVERITY GENMASK(1, 0)
> +static void cxl_cper_event_call(enum cxl_event_type ev_type,
> +				struct cxl_cper_event_rec *rec)
> +{
> +	struct cper_cxl_event_devid *device_id = &rec->hdr.device_id;
> +	struct pci_dev *pdev __free(pci_dev_put) = NULL;
> +	enum cxl_event_log_type log_type;
> +	struct cxl_dev_state *cxlds;
> +	unsigned int devfn;
> +	u32 hdr_flags;
> +
> +	pr_debug("CPER event for device %u:%u:%u.%u\n",
> +		 device_id->segment_num, device_id->bus_num,
> +		 device_id->device_num, device_id->func_num);
> +
> +	devfn = PCI_DEVFN(device_id->device_num, device_id->func_num);
> +	pdev = pci_get_domain_bus_and_slot(device_id->segment_num,
> +					   device_id->bus_num, devfn);
> +	if (!pdev) {
> +		pr_err("CPER event device %u:%u:%u.%u not found\n",
> +			device_id->segment_num, device_id->bus_num,
> +			device_id->device_num, device_id->func_num);
> +		return;
> +	}
> +
> +	dev_dbg(&pdev->dev, "Found device %u:%u.%u\n", device_id->bus_num,
> +		device_id->device_num, device_id->func_num);

These print statements are excessive. The dev_dbg() already encodes the
device BDF into the device name. The pr_err() is not actionable and
somewhat redundant with the default cper_estatus_print_section() print.

I would just delete all of them.

[PATCH 4/4] ras/events: Trace CXL CPER events even without the CXL stack loaded

[Ira Weiny]

If CXL is solely managed by firmware (including HDM configuration and
event processing via firmware first) it is possible to run the system
without the CXL software loaded.  In this case no CXL callback will be
loaded and CXL CPER errors will not be processed at all.

In this case memory device and region (HPA) information is missing but
omitting the error completely is not friendly for such a user.  Some
device information is available in the generic event which could prove
useful to a user.

Utilize the local work item to trace a generic CXL CPER event.

Duplicate the pattern of decoding the CXL event header to aid in adding
future trace points if needed.  This was an easy lift from the CXL trace
points.  But stop at header decoding only because this is an unlikely
configuration for the system.  Further decoding can be obtained with
user space tools or added later if needed.

Cc: Ard Biesheuvel <ardb@kernel.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Borislav Petkov <bp@alien8.de>
Suggested-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 drivers/acpi/apei/ghes.c |  5 ++-
 include/ras/ras_event.h  | 90 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 94 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index f433f4eae888..9ac323cbf195 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -729,7 +729,10 @@ static void cxl_cper_local_fn(struct work_struct *work)
 
 	while (kfifo_out_spinlocked(&cxl_cper_fifo, &wd, 1,
 			&cxl_cper_read_lock)) {
-		/* drop msg */
+		struct cxl_cper_event_rec *rec = &wd.rec;
+		union cxl_event *evt = &rec->event;
+
+		trace_cper_cxl_gen_event(rec, &evt->generic);
 	}
 }
 static DECLARE_WORK(cxl_local_work, cxl_cper_local_fn);
diff --git a/include/ras/ras_event.h b/include/ras/ras_event.h
index cbd3ddd7c33d..319faf552b65 100644
--- a/include/ras/ras_event.h
+++ b/include/ras/ras_event.h
@@ -422,6 +422,96 @@ TRACE_EVENT(memory_failure_event,
 	)
 );
 #endif /* CONFIG_MEMORY_FAILURE */
+
+#include <linux/cxl-event.h>
+#include <asm-generic/unaligned.h>
+
+/*
+ * Common Event Record Format
+ * CXL 3.0 section 8.2.9.2.1; Table 8-42
+ */
+#define CXL_EVENT_RECORD_FLAG_PERMANENT		BIT(2)
+#define CXL_EVENT_RECORD_FLAG_MAINT_NEEDED	BIT(3)
+#define CXL_EVENT_RECORD_FLAG_PERF_DEGRADED	BIT(4)
+#define CXL_EVENT_RECORD_FLAG_HW_REPLACE	BIT(5)
+#define show_hdr_flags(flags)	__print_flags(flags, " | ",			   \
+	{ CXL_EVENT_RECORD_FLAG_PERMANENT,	"PERMANENT_CONDITION"		}, \
+	{ CXL_EVENT_RECORD_FLAG_MAINT_NEEDED,	"MAINTENANCE_NEEDED"		}, \
+	{ CXL_EVENT_RECORD_FLAG_PERF_DEGRADED,	"PERFORMANCE_DEGRADED"		}, \
+	{ CXL_EVENT_RECORD_FLAG_HW_REPLACE,	"HARDWARE_REPLACEMENT_NEEDED"	}  \
+)
+
+/*
+ * Define macros for the common header of each CPER CXL event.
+ *
+ * Tracepoints using these macros must do 3 things:
+ *
+ *	1) Add CPER_CXL_EVT_TP_entry to TP_STRUCT__entry
+ *	2) Use CPER_CXL_EVT_TP_fast_assign within TP_fast_assign;
+ *	   pass the serial number and CXL event header
+ *	3) Use CPER_CXL_EVT_TP_printk() instead of TP_printk()
+ *
+ * See the generic_event tracepoint as an example.
+ */
+#define CPER_CXL_EVT_TP_entry					\
+	__field(u16, segment)					\
+	__field(u8, bus)					\
+	__field(u8, device)					\
+	__field(u8, func)					\
+	__field(u64, serial)					\
+	__field(u32, hdr_flags)					\
+	__field(u16, hdr_handle)				\
+	__field(u16, hdr_related_handle)			\
+	__field(u64, hdr_timestamp)				\
+	__field(u8, hdr_length)					\
+	__field(u8, hdr_maint_op_class)
+
+#define CPER_CXL_EVT_TP_fast_assign(cper_rec, evt_hdr)				\
+	__entry->segment = cper_rec->hdr.device_id.segment_num;			\
+	__entry->bus = cper_rec->hdr.device_id.bus_num;				\
+	__entry->device = cper_rec->hdr.device_id.device_num;			\
+	__entry->func = cper_rec->hdr.device_id.func_num;			\
+	__entry->serial = (((u64)cper_rec->hdr.dev_serial_num.upper_dw) << 32) |\
+			 cper_rec->hdr.dev_serial_num.lower_dw;			\
+	__entry->hdr_length = (evt_hdr).length;					\
+	__entry->hdr_flags = get_unaligned_le24((evt_hdr).flags);		\
+	__entry->hdr_handle = le16_to_cpu((evt_hdr).handle);			\
+	__entry->hdr_related_handle = le16_to_cpu((evt_hdr).related_handle);	\
+	__entry->hdr_timestamp = le64_to_cpu((evt_hdr).timestamp);		\
+	__entry->hdr_maint_op_class = (evt_hdr).maint_op_class
+
+#define CPER_CXL_EVT_TP_printk(fmt, ...)					\
+	TP_printk("device=%04x:%02x:%02x.%02x serial=%lld : time=%llu "		\
+		"len=%d flags='%s' handle=%x related_handle=%x "		\
+		"maint_op_class=%u : " fmt,					\
+		__entry->segment, __entry->bus, __entry->device, __entry->func,	\
+		__entry->serial,						\
+		__entry->hdr_timestamp, __entry->hdr_length,			\
+		show_hdr_flags(__entry->hdr_flags), __entry->hdr_handle,	\
+		__entry->hdr_related_handle, __entry->hdr_maint_op_class,	\
+		##__VA_ARGS__)
+
+TRACE_EVENT(cper_cxl_gen_event,
+
+	TP_PROTO(struct cxl_cper_event_rec *cper_rec,
+		 struct cxl_event_generic *gen_rec),
+
+	TP_ARGS(cper_rec, gen_rec),
+
+	TP_STRUCT__entry(
+		CPER_CXL_EVT_TP_entry
+		__array(u8, data, CXL_EVENT_RECORD_DATA_LENGTH)
+	),
+
+	TP_fast_assign(
+		CPER_CXL_EVT_TP_fast_assign(cper_rec, gen_rec->hdr);
+		memcpy(__entry->data, gen_rec->data, CXL_EVENT_RECORD_DATA_LENGTH);
+	),
+
+	CPER_CXL_EVT_TP_printk("%s",
+		__print_hex(__entry->data, CXL_EVENT_RECORD_DATA_LENGTH))
+);
+
 #endif /* _TRACE_HW_EVENT_MC_H */
 
 /* This part must be outside protection */

-- 
2.43.0

Re: [PATCH 4/4] ras/events: Trace CXL CPER events even without the CXL stack loaded

[Dan Williams]

Ira Weiny wrote:
> If CXL is solely managed by firmware (including HDM configuration and
> event processing via firmware first) it is possible to run the system
> without the CXL software loaded.  In this case no CXL callback will be
> loaded and CXL CPER errors will not be processed at all.
> 
> In this case memory device and region (HPA) information is missing but
> omitting the error completely is not friendly for such a user.  Some
> device information is available in the generic event which could prove
> useful to a user.
> 
> Utilize the local work item to trace a generic CXL CPER event.
> 
> Duplicate the pattern of decoding the CXL event header to aid in adding
> future trace points if needed.  This was an easy lift from the CXL trace
> points.  But stop at header decoding only because this is an unlikely
> configuration for the system.  Further decoding can be obtained with
> user space tools or added later if needed.
> 
> Cc: Ard Biesheuvel <ardb@kernel.org>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: Tony Luck <tony.luck@intel.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> ---
>  drivers/acpi/apei/ghes.c |  5 ++-
>  include/ras/ras_event.h  | 90 ++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 94 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index f433f4eae888..9ac323cbf195 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -729,7 +729,10 @@ static void cxl_cper_local_fn(struct work_struct *work)
>  
>  	while (kfifo_out_spinlocked(&cxl_cper_fifo, &wd, 1,
>  			&cxl_cper_read_lock)) {
> -		/* drop msg */
> +		struct cxl_cper_event_rec *rec = &wd.rec;
> +		union cxl_event *evt = &rec->event;
> +
> +		trace_cper_cxl_gen_event(rec, &evt->generic);

So it was confusing to read the empty stub function 2 patches back when this
change was coming, and basic reporting of CXL event does not need the
workqueue indirection. Note that EDAC triggers trace events directly in
the atomic notifier chain, so CXL could do the same.

>  static DECLARE_WORK(cxl_local_work, cxl_cper_local_fn);
> diff --git a/include/ras/ras_event.h b/include/ras/ras_event.h
> index cbd3ddd7c33d..319faf552b65 100644
> --- a/include/ras/ras_event.h
> +++ b/include/ras/ras_event.h

This is more heavywieght than I was expecting and defeats the purpose of
centralizing advanced decode in the CXL driver itself.

I would expect this to be just the tracing equivalent of the
ignore_section logic in cper_estatus_print_section().

Re: [PATCH 4/4] ras/events: Trace CXL CPER events even without the CXL stack loaded

[Ira Weiny]

Dan Williams wrote:
> Ira Weiny wrote:
> > 
> > diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> > index f433f4eae888..9ac323cbf195 100644
> > --- a/drivers/acpi/apei/ghes.c
> > +++ b/drivers/acpi/apei/ghes.c
> > @@ -729,7 +729,10 @@ static void cxl_cper_local_fn(struct work_struct *work)
> >  
> >  	while (kfifo_out_spinlocked(&cxl_cper_fifo, &wd, 1,
> >  			&cxl_cper_read_lock)) {
> > -		/* drop msg */
> > +		struct cxl_cper_event_rec *rec = &wd.rec;
> > +		union cxl_event *evt = &rec->event;
> > +
> > +		trace_cper_cxl_gen_event(rec, &evt->generic);
> 
> So it was confusing to read the empty stub function 2 patches back when this
> change was coming, and basic reporting of CXL event does not need the
> workqueue indirection. Note that EDAC triggers trace events directly in
> the atomic notifier chain, so CXL could do the same.
> 
> >  static DECLARE_WORK(cxl_local_work, cxl_cper_local_fn);
> > diff --git a/include/ras/ras_event.h b/include/ras/ras_event.h
> > index cbd3ddd7c33d..319faf552b65 100644
> > --- a/include/ras/ras_event.h
> > +++ b/include/ras/ras_event.h
> 
> This is more heavywieght than I was expecting and defeats the purpose of
> centralizing advanced decode in the CXL driver itself.

Fair enough but it is not that heavy IMO.  It was mostly lifted from the
CXL traces.

> 
> I would expect this to be just the tracing equivalent of the
> ignore_section logic in cper_estatus_print_section().

I think it is fair to give the user some information.  I'll redo this
patch to be first and drop the extra work item.

Ira