RE: [PATCH v5 4/6] cxl/events: Update DRAM Event Record to CXL spec rev 3.1

[Shiju Jose]

>-----Original Message-----
>From: Ira Weiny <ira.weiny@xxxxxxxxx>
>Sent: 10 January 2025 18:27
>To: Shiju Jose <shiju.jose@xxxxxxxxxx>; dave.jiang@xxxxxxxxx;
>dan.j.williams@xxxxxxxxx; Jonathan Cameron
><jonathan.cameron@xxxxxxxxxx>; alison.schofield@xxxxxxxxx;
>nifan.cxl@xxxxxxxxx; vishal.l.verma@xxxxxxxxx; ira.weiny@xxxxxxxxx;
>dave@xxxxxxxxxxxx; linux-cxl@xxxxxxxxxxxxxxx
>Cc: linux-kernel@xxxxxxxxxxxxxxx; Linuxarm <linuxarm@xxxxxxxxxx>;
>tanxiaofei <tanxiaofei@xxxxxxxxxx>; Zengtao (B) <prime.zeng@xxxxxxxxxxxxx>;
>Shiju Jose <shiju.jose@xxxxxxxxxx>
>Subject: Re: [PATCH v5 4/6] cxl/events: Update DRAM Event Record to CXL spec
>rev 3.1
>
>shiju.jose@ wrote:
>> From: Shiju Jose <shiju.jose@xxxxxxxxxx>
>>
>
>[snip]
>
>>
>>  TRACE_EVENT(cxl_dram,
>> @@ -527,6 +539,7 @@ TRACE_EVENT(cxl_dram,
>>  		__field(u64, dpa)
>>  		__field(u8, descriptor)
>>  		__field(u8, type)
>> +		__field(u8, sub_type)
>
>I just noticed this with the previous patch too.  To pack the record this should be
>below...

Hi Ira,

Thanks for reviewing and for the feedbacks. 
I have modified for your suggestions and will be in the v6.

Thanks,
Shiju

>
>>  		__field(u8, transaction_type)
>>  		__field(u8, channel)
>>  		__field(u16, validity_flags)
>> @@ -541,6 +554,10 @@ TRACE_EVENT(cxl_dram,
>>  		__field(u8, bank)	/* Out of order to pack trace record */
>>  		__field(u8, dpa_flags)	/* Out of order to pack trace record */
>
>Here.
>
>>  		__string(region_name, cxlr ? dev_name(&cxlr->dev) : "")
>> +		__array(u8, comp_id, CXL_EVENT_GEN_MED_COMP_ID_SIZE)
>> +		__field(u8, sub_channel)
>> +		__field(u8, cme_threshold_ev_flags)
>> +		__field(u32, cvme_count)
>>  	),
>
>And these reordered too.  Like this:
>
>diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h index
>cbaf6244d77f..6f4bf4925cbf 100644
>--- a/drivers/cxl/core/trace.h
>+++ b/drivers/cxl/core/trace.h
>@@ -539,7 +539,6 @@ TRACE_EVENT(cxl_dram,
>                __field(u64, dpa)
>                __field(u8, descriptor)
>                __field(u8, type)
>-               __field(u8, sub_type)
>                __field(u8, transaction_type)
>                __field(u8, channel)
>                __field(u16, validity_flags) @@ -553,11 +552,13 @@
>TRACE_EVENT(cxl_dram,
>                __field(u8, bank_group) /* Out of order to pack trace record */
>                __field(u8, bank)       /* Out of order to pack trace record */
>                __field(u8, dpa_flags)  /* Out of order to pack trace record */
>-               __string(region_name, cxlr ? dev_name(&cxlr->dev) : "")
>+               /* The following are out of order to pack the trace
>+ record */
>                __array(u8, comp_id, CXL_EVENT_GEN_MED_COMP_ID_SIZE)
>+               __field(u32, cvme_count)
>+               __field(u8, sub_type)
>                __field(u8, sub_channel)
>                __field(u8, cme_threshold_ev_flags)
>-               __field(u32, cvme_count)
>+               __string(region_name, cxlr ? dev_name(&cxlr->dev) : "")
>        ),
>
>        TP_fast_assign(
>
>
>
>Other than that it looks good.  Perhaps Dave can just squash this hunk?
>
>Ira
>
>[snip]