RE: [RFC PATCH 2/4] cxl/events: Updates for CXL General Media Event Record

[Shiju Jose]

>-----Original Message-----
>From: Jonathan Cameron <jonathan.cameron@xxxxxxxxxx>
>Sent: 17 October 2024 13:25
>To: Shiju Jose <shiju.jose@xxxxxxxxxx>
>Cc: dave.jiang@xxxxxxxxx; dan.j.williams@xxxxxxxxx; alison.schofield@xxxxxxxxx;
>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 2/4] cxl/events: Updates for CXL General Media Event
>Record
>
>On Wed, 16 Oct 2024 17:33:47 +0100
><shiju.jose@xxxxxxxxxx> wrote:
>
>> From: Shiju Jose <shiju.jose@xxxxxxxxxx>
>>
>> CXL spec rev 3.1 section 8.2.9.2.1.1 Table 8-45, General Media Event
>> Record has updated with following new fields and new types for Memory
>> Event Type and Transaction Type fields.
>> 1. Advanced Programmable Corrected Memory Error Threshold Event Flags
>> 2. Corrected Memory Error Count at Event 3. Memory Event Sub-Type
>>
>> The component identifier format has changed (CXL spec 3.1 section
>> 8.2.9.2.1 Table 8-44).
>>
>> Add updates for the above spec changes in the CXL events record and
>> CXL general media trace event implementations.
>>
>> Signed-off-by: Shiju Jose <shiju.jose@xxxxxxxxxx>
>
>It might be worth breaking out the component ID formatting as a separate
>patch.  That comes in 3.1 along with the other fields but is perhaps more
>controversial?
I will add separate patch for component ID formatting.

>
>It's a good change, but will change what is printed. I 'think'
>tracepoint printing is typically not considered ABI though (unlike the tracepoint
>which is!) so should be fine.
>
>Split or not I like the component ID formatting and the reset LGTM I also slight
>prefer the fact you inserted new fields in logical places (so disagree with Alison
>if that's what she meant).
>Good to call that out in the patch description though to highlight it as something
>people might want to consider.
Sure.
>
>Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
>
>
>> ---
>> Question:
>> Want more abbreviations for the long lines of code in
>> show_mem_event_sub_type() and for similar in other patches?
>I raised this in internal review, but don't think it matters that much either way :)
Ok.

Thanks,
Shiju