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

Shiju Jose
2025-01-11T09:16:11Z

>-----Original Message-----
>From: Ira Weiny <ira.weiny@intel.com>
>Sent: 10 January 2025 18:27
>To: Shiju Jose <shiju.jose@huawei.com>; dave.jiang@intel.com;
>dan.j.williams@intel.com; Jonathan Cameron
><jonathan.cameron@huawei.com>; alison.schofield@intel.com;
>nifan.cxl@gmail.com; vishal.l.verma@intel.com; ira.weiny@intel.com;
>dave@stgolabs.net; linux-cxl@vger.kernel.org
>Cc: linux-kernel@vger.kernel.org; Linuxarm <linuxarm@huawei.com>;
>tanxiaofei <tanxiaofei@huawei.com>; Zengtao (B) <prime.zeng@hisilicon.com>;
>Shiju Jose <shiju.jose@huawei.com>
>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@huawei.com>
>>
>
>[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]