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
