RE: [RFC PATCH 0/4] Updates for CXL Event Records

[Shiju Jose]

>-----Original Message-----
>From: Alison Schofield <alison.schofield@xxxxxxxxx>
>Sent: 16 October 2024 22:01
>To: Shiju Jose <shiju.jose@xxxxxxxxxx>
>Cc: dave.jiang@xxxxxxxxx; dan.j.williams@xxxxxxxxx; Jonathan Cameron
><jonathan.cameron@xxxxxxxxxx>; vishal.l.verma@xxxxxxxxx;
>ira.weiny@xxxxxxxxx; dave@xxxxxxxxxxxx; linux-cxl@xxxxxxxxxxxxxxx; linux-
>kernel@xxxxxxxxxxxxxxx; Linuxarm <linuxarm@xxxxxxxxxx>; tanxiaofei
><tanxiaofei@xxxxxxxxxx>; Zengtao (B) <prime.zeng@xxxxxxxxxxxxx>
>Subject: Re: [RFC PATCH 0/4] Updates for CXL Event Records
>
>On Wed, Oct 16, 2024 at 05:33:45PM +0100, shiju.jose@xxxxxxxxxx wrote:
>> From: Shiju Jose <shiju.jose@xxxxxxxxxx>
>>
>> CXL spec rev 3.1 CXL Event Records has updated w.r.t CXL spec rev 3.0.
>> Add updates for the above spec changes in the CXL events records and
>> CXL trace events implementation.
>>
>> Note: Please apply following fix patch first if not present.
>> https://patchwork.kernel.org/project/cxl/patch/20241014143003.1170-1-s
>> hiju.jose@xxxxxxxxxx/
>>
>> Shiju Jose (4):
>>   cxl/events: Updates for CXL Common Event Record Format
>>   cxl/events: Updates for CXL General Media Event Record
>>   cxl/events: Updates for CXL DRAM Event Record
>>   cxl/events: Updates for CXL Memory Module Event Record
>
>Thanks, this looks useful! I didn't review line by line but do have some feedback
>before for a v1:
Hi Alison,
Thanks for the feedbacks.

>
>- Suggest being more explicit in the commit msg(s). Something like:
>cxl/events: Update Common Event Record to CXL spec 3.1
Sure. Will add.

>
>- I was a bit surprised that this doesn't simply append new fields to the
>TP_printk() output. Is there some reason for that?
Will do. Thought print new fields before region_name and region_uuid. 

>
>- How about updating the mock of these events to include these new fields. I
>don't think this introduces any new formats, but I would certainly eyeball all 3:
>dmesg tp_printk, trace file, and monitor output because all 3 (sadly) present a
>bit differently.
>
I will update the CXL mock test for these new fields.
 
>-- Alison
>
>>
>>  drivers/cxl/core/trace.h | 201 +++++++++++++++++++++++++++++++++------
>>  include/cxl/event.h      |  20 +++-
>>  2 files changed, 190 insertions(+), 31 deletions(-)
>>
>> --
>> 2.34.1
>>
>
Thanks,
Shiju