diff mbox series

[1/4] acpi/ghes, cxl: Create a common CXL struct to handle different CXL CPER records

Message ID 20240102150933.161009-2-Smita.KoralahalliChannabasappa@amd.com
State New
Headers
Series acpi/ghes, cper, cxl: Trace FW-First CXL Protocol Errors |

Commit Message

Smita Koralahalli Jan. 2, 2024, 3:09 p.m. UTC 
  Currently defined cxl_cper_callback interface between CXL subsystem and
GHES module is just confined to handling CXL Component errors only.

Extend this callback to process CXL Protocol errors as well. Achieve
by defining a new struct cxl_cper_rec_data to include cxl_cper_event_rec
and other fields of CXL protocol errors which will be defined in future
patches.

Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
---
 drivers/acpi/apei/ghes.c  | 6 +++++-
 drivers/cxl/pci.c         | 8 ++++----
 include/linux/cxl-event.h | 6 +++++-
 3 files changed, 14 insertions(+), 6 deletions(-)

Comments

Ira Weiny Jan. 2, 2024, 4:23 p.m. UTC | #1 
Smita Koralahalli wrote:
> Currently defined cxl_cper_callback interface between CXL subsystem and
> GHES module is just confined to handling CXL Component errors only.
> 
> Extend this callback to process CXL Protocol errors as well. Achieve
> by defining a new struct cxl_cper_rec_data to include cxl_cper_event_rec
> and other fields of CXL protocol errors which will be defined in future
> patches.
> 
> Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>

[snip]

>  
>  static int __init cxl_pci_driver_init(void)
> diff --git a/include/linux/cxl-event.h b/include/linux/cxl-event.h
> index 17eadee819b6..afa71ee0437c 100644
> --- a/include/linux/cxl-event.h
> +++ b/include/linux/cxl-event.h
> @@ -141,8 +141,12 @@ struct cxl_cper_event_rec {
>  	union cxl_event event;
>  } __packed;
>  
> +struct cxl_cper_rec_data {
> +	struct cxl_cper_event_rec rec;

NIT: I would call this something like event to distinguish it from other
record data.

Other than that this seems reasonable to me.

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

[snip]
Smita Koralahalli Jan. 3, 2024, 8:04 p.m. UTC | #2 
On 1/2/2024 8:23 AM, Ira Weiny wrote:
> Smita Koralahalli wrote:
>> Currently defined cxl_cper_callback interface between CXL subsystem and
>> GHES module is just confined to handling CXL Component errors only.
>>
>> Extend this callback to process CXL Protocol errors as well. Achieve
>> by defining a new struct cxl_cper_rec_data to include cxl_cper_event_rec
>> and other fields of CXL protocol errors which will be defined in future
>> patches.
>>
>> Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
> 
> [snip]
> 
>>   
>>   static int __init cxl_pci_driver_init(void)
>> diff --git a/include/linux/cxl-event.h b/include/linux/cxl-event.h
>> index 17eadee819b6..afa71ee0437c 100644
>> --- a/include/linux/cxl-event.h
>> +++ b/include/linux/cxl-event.h
>> @@ -141,8 +141,12 @@ struct cxl_cper_event_rec {
>>   	union cxl_event event;
>>   } __packed;
>>   
>> +struct cxl_cper_rec_data {
>> +	struct cxl_cper_event_rec rec;
> 
> NIT: I would call this something like event to distinguish it from other
> record data.

What do you think of the below?

struct cxl_cper_event_info {
	struct cxl_cper_event_rec rec;
	struct cxl_cper_prot_err {
		struct cxl_ras_capability_regs cxl_ras;
		int severity;
	} p_err;
};

Addressed changing to sub-struct and copying the struct rather than 
pointer comments in patch 3..

> 
> Other than that this seems reasonable to me.
> 
> Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> 
> [snip]
>
Ira Weiny Jan. 3, 2024, 8:35 p.m. UTC | #3 
Smita Koralahalli wrote:
> On 1/2/2024 8:23 AM, Ira Weiny wrote:
> > Smita Koralahalli wrote:

[snip]

> >> diff --git a/include/linux/cxl-event.h b/include/linux/cxl-event.h
> >> index 17eadee819b6..afa71ee0437c 100644
> >> --- a/include/linux/cxl-event.h
> >> +++ b/include/linux/cxl-event.h
> >> @@ -141,8 +141,12 @@ struct cxl_cper_event_rec {
> >>   	union cxl_event event;
> >>   } __packed;
> >>   
> >> +struct cxl_cper_rec_data {
> >> +	struct cxl_cper_event_rec rec;
> > 
> > NIT: I would call this something like event to distinguish it from other
> > record data.
> 
> What do you think of the below?
> 
> struct cxl_cper_event_info {
> 	struct cxl_cper_event_rec rec;
> 	struct cxl_cper_prot_err {
> 		struct cxl_ras_capability_regs cxl_ras;
> 		int severity;
> 	} p_err;
> };
> 
> Addressed changing to sub-struct and copying the struct rather than 
> pointer comments in patch 3..

That is much better.  Thanks!
Ira
diff mbox series

Patch

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index aed465d2fd68..d6a85fbc0a8b 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -693,6 +693,10 @@  static cxl_cper_callback cper_callback;
 static void cxl_cper_post_event(enum cxl_event_type event_type,
 				struct cxl_cper_event_rec *rec)
 {
+	struct cxl_cper_rec_data data;
+
+	data.rec = *(struct cxl_cper_event_rec *)rec;
+
 	if (rec->hdr.length <= sizeof(rec->hdr) ||
 	    rec->hdr.length > sizeof(*rec)) {
 		pr_err(FW_WARN "CXL CPER Invalid section length (%u)\n",
@@ -707,7 +711,7 @@  static void cxl_cper_post_event(enum cxl_event_type event_type,
 
 	guard(rwsem_read)(&cxl_cper_rw_sem);
 	if (cper_callback)
-		cper_callback(event_type, rec);
+		cper_callback(event_type, &data);
 }
 
 int cxl_cper_register_callback(cxl_cper_callback callback)
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index b14237f824cf..7edbd53357e5 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -972,9 +972,9 @@  static struct 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 cxl_cper_rec_data *data)
 {
-	struct cper_cxl_event_devid *device_id = &rec->hdr.device_id;
+	struct cper_cxl_event_devid *device_id = &data->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;
@@ -996,11 +996,11 @@  static void cxl_cper_event_call(enum cxl_event_type ev_type,
 		return;
 
 	/* Fabricate a log type */
-	hdr_flags = get_unaligned_le24(rec->event.generic.hdr.flags);
+	hdr_flags = get_unaligned_le24(data->rec.event.generic.hdr.flags);
 	log_type = FIELD_GET(CXL_EVENT_HDR_FLAGS_REC_SEVERITY, hdr_flags);
 
 	cxl_event_trace_record(cxlds->cxlmd, log_type, ev_type,
-			       &uuid_null, &rec->event);
+			       &uuid_null, &data->rec.event);
 }
 
 static int __init cxl_pci_driver_init(void)
diff --git a/include/linux/cxl-event.h b/include/linux/cxl-event.h
index 17eadee819b6..afa71ee0437c 100644
--- a/include/linux/cxl-event.h
+++ b/include/linux/cxl-event.h
@@ -141,8 +141,12 @@  struct cxl_cper_event_rec {
 	union cxl_event event;
 } __packed;
 
+struct cxl_cper_rec_data {
+	struct cxl_cper_event_rec rec;
+};
+
 typedef void (*cxl_cper_callback)(enum cxl_event_type type,
-				  struct cxl_cper_event_rec *rec);
+				  struct cxl_cper_rec_data *data);
 
 #ifdef CONFIG_ACPI_APEI_GHES
 int cxl_cper_register_callback(cxl_cper_callback callback);