| Message ID | 20230601-cxl-cper-v1-1-d19f1ac18ab6@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | efi/cxl-cper: Report CPER CXL component events through trace events | expand |
Ira Weiny wrote: > CPER CXL events do not have a UUID associated with them. It is > desirable to share event structures between the CPER CXL event and the > CXL event log events. This parses strange to me, maybe it is trying to capture too many changes in too few sentences? Something like this instead? "CXL CPER events are identified by the CPER Section Type GUID. The GUID correlates with the CXL UUID for the event record. It turns out that a CXL CPER record is a strict subset of the CXL event record, only the UUID header field is chopped. In order to unify handling between native and CPER flavors of CXL events, prepare the code for the UUID to be passed in rather than inferred from the record itself. Later patches update the passed in record to only refer to the common data between the formats." > > Pass the UUID explicitly to each trace event to be able to remove the > UUID from the event structures. > > Originally it was desirable to remove the UUID from the well known event > because the UUID value was redundant. However, the trace API was > already in place.[1] > > [1] https://lore.kernel.org/all/36f2d12934d64a278f2c0313cbd01abc@huawei.com/ > > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > --- > drivers/cxl/core/mbox.c | 8 ++++---- > drivers/cxl/core/trace.h | 32 ++++++++++++++++---------------- > 2 files changed, 20 insertions(+), 20 deletions(-) > > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c > index 36270dcfb42e..00f429c440df 100644 > --- a/drivers/cxl/core/mbox.c > +++ b/drivers/cxl/core/mbox.c > @@ -870,19 +870,19 @@ static void cxl_event_trace_record(const struct cxl_memdev *cxlmd, > struct cxl_event_gen_media *rec = > (struct cxl_event_gen_media *)record; > > - trace_cxl_general_media(cxlmd, type, rec); > + trace_cxl_general_media(cxlmd, type, id, rec); > } else if (uuid_equal(id, &dram_event_uuid)) { > struct cxl_event_dram *rec = (struct cxl_event_dram *)record; > > - trace_cxl_dram(cxlmd, type, rec); > + trace_cxl_dram(cxlmd, type, id, rec); > } else if (uuid_equal(id, &mem_mod_event_uuid)) { > struct cxl_event_mem_module *rec = > (struct cxl_event_mem_module *)record; > > - trace_cxl_memory_module(cxlmd, type, rec); > + trace_cxl_memory_module(cxlmd, type, id, rec); > } else { > /* For unknown record types print just the header */ > - trace_cxl_generic_event(cxlmd, type, record); > + trace_cxl_generic_event(cxlmd, type, id, record); > } > } > > diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h > index a0b5819bc70b..2aef185f4cd0 100644 > --- a/drivers/cxl/core/trace.h > +++ b/drivers/cxl/core/trace.h > @@ -189,7 +189,7 @@ TRACE_EVENT(cxl_overflow, > __string(memdev, dev_name(&cxlmd->dev)) \ > __string(host, dev_name(cxlmd->dev.parent)) \ > __field(int, log) \ > - __field_struct(uuid_t, hdr_uuid) \ > + __field_struct(uuid_t, uuid) \ Hmm, is it too late to make this rename? I.e. would a script that is looking for "hdr_uuid" in one kernel be broken by the rename of the field to "uuid" in the next? > __field(u64, serial) \ > __field(u32, hdr_flags) \ > __field(u16, hdr_handle) \ > @@ -198,12 +198,12 @@ TRACE_EVENT(cxl_overflow, > __field(u8, hdr_length) \ > __field(u8, hdr_maint_op_class) > > -#define CXL_EVT_TP_fast_assign(cxlmd, l, hdr) \ > +#define CXL_EVT_TP_fast_assign(cxlmd, l, uuid, hdr) \ > __assign_str(memdev, dev_name(&(cxlmd)->dev)); \ > __assign_str(host, dev_name((cxlmd)->dev.parent)); \ > __entry->log = (l); \ > __entry->serial = (cxlmd)->cxlds->serial; \ > - memcpy(&__entry->hdr_uuid, &(hdr).id, sizeof(uuid_t)); \ > + memcpy(&__entry->uuid, (uuid), sizeof(uuid_t)); \ Per above this would be: memcpy(&__entry->hdr_uuid, (uuid), sizeof(uuid_t)); \ > __entry->hdr_length = (hdr).length; \ > __entry->hdr_flags = get_unaligned_le24((hdr).flags); \ > __entry->hdr_handle = le16_to_cpu((hdr).handle); \ > @@ -217,7 +217,7 @@ TRACE_EVENT(cxl_overflow, > "maint_op_class=%u : " fmt, \ > __get_str(memdev), __get_str(host), __entry->serial, \ > cxl_event_log_type_str(__entry->log), \ > - __entry->hdr_timestamp, &__entry->hdr_uuid, __entry->hdr_length,\ > + __entry->hdr_timestamp, &__entry->uuid, __entry->hdr_length, \ ...and this change could be dropped. Other than that, this looks good to me.
Dan Williams wrote: > Ira Weiny wrote: > > CPER CXL events do not have a UUID associated with them. It is > > desirable to share event structures between the CPER CXL event and the > > CXL event log events. > > This parses strange to me, maybe it is trying to capture too many > changes in too few sentences? Something like this instead? Yea probably just trying to squeeze too much in. > > "CXL CPER events are identified by the CPER Section Type GUID. The GUID > correlates with the CXL UUID for the event record. It turns out that a > CXL CPER record is a strict subset of the CXL event record, only the > UUID header field is chopped. > > In order to unify handling between native and CPER flavors of CXL > events, prepare the code for the UUID to be passed in rather than > inferred from the record itself. > > Later patches update the passed in record to only refer to the common > data between the formats." That looks much more detailed. I'll add it in. [snip] > > diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h > > index a0b5819bc70b..2aef185f4cd0 100644 > > --- a/drivers/cxl/core/trace.h > > +++ b/drivers/cxl/core/trace.h > > @@ -189,7 +189,7 @@ TRACE_EVENT(cxl_overflow, > > __string(memdev, dev_name(&cxlmd->dev)) \ > > __string(host, dev_name(cxlmd->dev.parent)) \ > > __field(int, log) \ > > - __field_struct(uuid_t, hdr_uuid) \ > > + __field_struct(uuid_t, uuid) \ > > Hmm, is it too late to make this rename? I.e. would a script that is > looking for "hdr_uuid" in one kernel be broken by the rename of the > field to "uuid" in the next? True, probably best to leave the field name alone. I'll change it back. > > > __field(u64, serial) \ > > __field(u32, hdr_flags) \ > > __field(u16, hdr_handle) \ > > @@ -198,12 +198,12 @@ TRACE_EVENT(cxl_overflow, > > __field(u8, hdr_length) \ > > __field(u8, hdr_maint_op_class) > > > > -#define CXL_EVT_TP_fast_assign(cxlmd, l, hdr) \ > > +#define CXL_EVT_TP_fast_assign(cxlmd, l, uuid, hdr) \ > > __assign_str(memdev, dev_name(&(cxlmd)->dev)); \ > > __assign_str(host, dev_name((cxlmd)->dev.parent)); \ > > __entry->log = (l); \ > > __entry->serial = (cxlmd)->cxlds->serial; \ > > - memcpy(&__entry->hdr_uuid, &(hdr).id, sizeof(uuid_t)); \ > > + memcpy(&__entry->uuid, (uuid), sizeof(uuid_t)); \ > > Per above this would be: > > memcpy(&__entry->hdr_uuid, (uuid), sizeof(uuid_t)); \ yep. > > > __entry->hdr_length = (hdr).length; \ > > __entry->hdr_flags = get_unaligned_le24((hdr).flags); \ > > __entry->hdr_handle = le16_to_cpu((hdr).handle); \ > > @@ -217,7 +217,7 @@ TRACE_EVENT(cxl_overflow, > > "maint_op_class=%u : " fmt, \ > > __get_str(memdev), __get_str(host), __entry->serial, \ > > cxl_event_log_type_str(__entry->log), \ > > - __entry->hdr_timestamp, &__entry->hdr_uuid, __entry->hdr_length,\ > > + __entry->hdr_timestamp, &__entry->uuid, __entry->hdr_length, \ > > ...and this change could be dropped. Yep. All done. > > Other than that, this looks good to me. Thanks, Ira
diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c index 36270dcfb42e..00f429c440df 100644 --- a/drivers/cxl/core/mbox.c +++ b/drivers/cxl/core/mbox.c @@ -870,19 +870,19 @@ static void cxl_event_trace_record(const struct cxl_memdev *cxlmd, struct cxl_event_gen_media *rec = (struct cxl_event_gen_media *)record; - trace_cxl_general_media(cxlmd, type, rec); + trace_cxl_general_media(cxlmd, type, id, rec); } else if (uuid_equal(id, &dram_event_uuid)) { struct cxl_event_dram *rec = (struct cxl_event_dram *)record; - trace_cxl_dram(cxlmd, type, rec); + trace_cxl_dram(cxlmd, type, id, rec); } else if (uuid_equal(id, &mem_mod_event_uuid)) { struct cxl_event_mem_module *rec = (struct cxl_event_mem_module *)record; - trace_cxl_memory_module(cxlmd, type, rec); + trace_cxl_memory_module(cxlmd, type, id, rec); } else { /* For unknown record types print just the header */ - trace_cxl_generic_event(cxlmd, type, record); + trace_cxl_generic_event(cxlmd, type, id, record); } } diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h index a0b5819bc70b..2aef185f4cd0 100644 --- a/drivers/cxl/core/trace.h +++ b/drivers/cxl/core/trace.h @@ -189,7 +189,7 @@ TRACE_EVENT(cxl_overflow, __string(memdev, dev_name(&cxlmd->dev)) \ __string(host, dev_name(cxlmd->dev.parent)) \ __field(int, log) \ - __field_struct(uuid_t, hdr_uuid) \ + __field_struct(uuid_t, uuid) \ __field(u64, serial) \ __field(u32, hdr_flags) \ __field(u16, hdr_handle) \ @@ -198,12 +198,12 @@ TRACE_EVENT(cxl_overflow, __field(u8, hdr_length) \ __field(u8, hdr_maint_op_class) -#define CXL_EVT_TP_fast_assign(cxlmd, l, hdr) \ +#define CXL_EVT_TP_fast_assign(cxlmd, l, uuid, hdr) \ __assign_str(memdev, dev_name(&(cxlmd)->dev)); \ __assign_str(host, dev_name((cxlmd)->dev.parent)); \ __entry->log = (l); \ __entry->serial = (cxlmd)->cxlds->serial; \ - memcpy(&__entry->hdr_uuid, &(hdr).id, sizeof(uuid_t)); \ + memcpy(&__entry->uuid, (uuid), sizeof(uuid_t)); \ __entry->hdr_length = (hdr).length; \ __entry->hdr_flags = get_unaligned_le24((hdr).flags); \ __entry->hdr_handle = le16_to_cpu((hdr).handle); \ @@ -217,7 +217,7 @@ TRACE_EVENT(cxl_overflow, "maint_op_class=%u : " fmt, \ __get_str(memdev), __get_str(host), __entry->serial, \ cxl_event_log_type_str(__entry->log), \ - __entry->hdr_timestamp, &__entry->hdr_uuid, __entry->hdr_length,\ + __entry->hdr_timestamp, &__entry->uuid, __entry->hdr_length, \ show_hdr_flags(__entry->hdr_flags), __entry->hdr_handle, \ __entry->hdr_related_handle, __entry->hdr_maint_op_class, \ ##__VA_ARGS__) @@ -225,9 +225,9 @@ TRACE_EVENT(cxl_overflow, TRACE_EVENT(cxl_generic_event, TP_PROTO(const struct cxl_memdev *cxlmd, enum cxl_event_log_type log, - struct cxl_event_record_raw *rec), + const uuid_t *uuid, struct cxl_event_record_raw *rec), - TP_ARGS(cxlmd, log, rec), + TP_ARGS(cxlmd, log, uuid, rec), TP_STRUCT__entry( CXL_EVT_TP_entry @@ -235,7 +235,7 @@ TRACE_EVENT(cxl_generic_event, ), TP_fast_assign( - CXL_EVT_TP_fast_assign(cxlmd, log, rec->hdr); + CXL_EVT_TP_fast_assign(cxlmd, log, uuid, rec->hdr); memcpy(__entry->data, &rec->data, CXL_EVENT_RECORD_DATA_LENGTH); ), @@ -315,9 +315,9 @@ TRACE_EVENT(cxl_generic_event, TRACE_EVENT(cxl_general_media, TP_PROTO(const struct cxl_memdev *cxlmd, enum cxl_event_log_type log, - struct cxl_event_gen_media *rec), + const uuid_t *uuid, struct cxl_event_gen_media *rec), - TP_ARGS(cxlmd, log, rec), + TP_ARGS(cxlmd, log, uuid, rec), TP_STRUCT__entry( CXL_EVT_TP_entry @@ -336,7 +336,7 @@ TRACE_EVENT(cxl_general_media, ), TP_fast_assign( - CXL_EVT_TP_fast_assign(cxlmd, log, rec->hdr); + CXL_EVT_TP_fast_assign(cxlmd, log, uuid, rec->hdr); /* General Media */ __entry->dpa = le64_to_cpu(rec->phys_addr); @@ -398,9 +398,9 @@ TRACE_EVENT(cxl_general_media, TRACE_EVENT(cxl_dram, TP_PROTO(const struct cxl_memdev *cxlmd, enum cxl_event_log_type log, - struct cxl_event_dram *rec), + const uuid_t *uuid, struct cxl_event_dram *rec), - TP_ARGS(cxlmd, log, rec), + TP_ARGS(cxlmd, log, uuid, rec), TP_STRUCT__entry( CXL_EVT_TP_entry @@ -422,7 +422,7 @@ TRACE_EVENT(cxl_dram, ), TP_fast_assign( - CXL_EVT_TP_fast_assign(cxlmd, log, rec->hdr); + CXL_EVT_TP_fast_assign(cxlmd, log, uuid, rec->hdr); /* DRAM */ __entry->dpa = le64_to_cpu(rec->phys_addr); @@ -547,9 +547,9 @@ TRACE_EVENT(cxl_dram, TRACE_EVENT(cxl_memory_module, TP_PROTO(const struct cxl_memdev *cxlmd, enum cxl_event_log_type log, - struct cxl_event_mem_module *rec), + const uuid_t *uuid, struct cxl_event_mem_module *rec), - TP_ARGS(cxlmd, log, rec), + TP_ARGS(cxlmd, log, uuid, rec), TP_STRUCT__entry( CXL_EVT_TP_entry @@ -569,7 +569,7 @@ TRACE_EVENT(cxl_memory_module, ), TP_fast_assign( - CXL_EVT_TP_fast_assign(cxlmd, log, rec->hdr); + CXL_EVT_TP_fast_assign(cxlmd, log, uuid, rec->hdr); /* Memory Module Event */ __entry->event_type = rec->event_type;
CPER CXL events do not have a UUID associated with them. It is desirable to share event structures between the CPER CXL event and the CXL event log events. Pass the UUID explicitly to each trace event to be able to remove the UUID from the event structures. Originally it was desirable to remove the UUID from the well known event because the UUID value was redundant. However, the trace API was already in place.[1] [1] https://lore.kernel.org/all/36f2d12934d64a278f2c0313cbd01abc@huawei.com/ Signed-off-by: Ira Weiny <ira.weiny@intel.com> --- drivers/cxl/core/mbox.c | 8 ++++---- drivers/cxl/core/trace.h | 32 ++++++++++++++++---------------- 2 files changed, 20 insertions(+), 20 deletions(-)