RE: [PATCH v2 2/6] cxl/events: Add Component Identifier formatting for CXL spec rev 3.1

[Shiju Jose <shiju.jose@huawei.com>]

>-----Original Message-----
>From: Jonathan Cameron <jonathan.cameron@huawei.com>
>Sent: 22 October 2024 17:58
>To: Shiju Jose <shiju.jose@huawei.com>
>Cc: dave.jiang@intel.com; dan.j.williams@intel.com; alison.schofield@intel.com;
>vishal.l.verma@intel.com; ira.weiny@intel.com; dave@stgolabs.net; linux-
>cxl@vger.kernel.org; linux-kernel@vger.kernel.org; Linuxarm
><linuxarm@huawei.com>; tanxiaofei <tanxiaofei@huawei.com>; Zengtao (B)
><prime.zeng@hisilicon.com>
>Subject: Re: [PATCH v2 2/6] cxl/events: Add Component Identifier formatting for
>CXL spec rev 3.1
>
>On Tue, 22 Oct 2024 11:58:45 +0100
><shiju.jose@huawei.com> wrote:
>
>> From: Shiju Jose <shiju.jose@huawei.com>
>>
>> Add Component Identifier formatting for CXL spec rev 3.1, Section
>> 8.2.9.2.1, Table 8-44.
>>
>> Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
>> ---
>>  drivers/cxl/core/trace.h | 20 ++++++++++++++++++++
>>  1 file changed, 20 insertions(+)
>>
>> diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h index
>> 7305974e2301..81908072c6f3 100644
>> --- a/drivers/cxl/core/trace.h
>> +++ b/drivers/cxl/core/trace.h
>> @@ -268,6 +268,26 @@ TRACE_EVENT(cxl_generic_event,
>>  	{ CXL_DPA_NOT_REPAIRABLE,		"NOT_REPAIRABLE"	}  \
>>  )
>>
>> +/*
>> + * Component ID Format
>> + * CXL 3.1 section 8.2.9.2.1; Table 8-44  */
>> +#define CXL_PLDM_COMPONENT_ID_ENTITY_VALID	BIT(0)
>> +#define CXL_PLDM_COMPONENT_ID_RES_VALID		BIT(1)
>> +
>> +#define comp_id_pldm_flags(flags)  __print_flags(flags, " | ",
>	\
>> +	{ CXL_PLDM_COMPONENT_ID_ENTITY_VALID,   "PLDM_Entity_ID[5:0] "
>},	\
>> +	{ CXL_PLDM_COMPONENT_ID_RES_VALID,      "Resource_ID[9:6] " }
>		\
>> +)
>> +
>> +#define cxl_print_component_id(flags, valid_comp_id, valid_id_format,
>comp_id)			\
>> +	(flags & valid_comp_id && flags & valid_id_format) ?
>			\
>> +	(comp_id[0] & (CXL_PLDM_COMPONENT_ID_ENTITY_VALID |
>CXL_PLDM_COMPONENT_ID_RES_VALID)) ?	\
>> +	__print_hex(&comp_id[1], 10) : (comp_id[0] &
>CXL_PLDM_COMPONENT_ID_ENTITY_VALID) ?	\
>> +	__print_hex(&comp_id[1], 6) : (comp_id[0] &
>CXL_PLDM_COMPONENT_ID_RES_VALID) ?		\
>> +	__print_hex(&comp_id[7], 4) : __print_hex(comp_id,
>CXL_EVENT_GEN_MED_COMP_ID_SIZE) :	\
>
>> +	__print_hex(comp_id, CXL_EVENT_GEN_MED_COMP_ID_SIZE)
>> +
>Hi Shiju,
>
>This is hard to read and I've lost track of what the aim is.
>Side note that it is probably good to state that in the patch description
>+ give some examples of what this print looks like in the various cases.
>If we are going to print the whole thing in the case where we have no valid bits
>set in byte[1], maybe we just print the whole thing in all cases and just add the
>info on whether it is formatted and what those valid bits are in byte[1]?
>
>I was never keen on printing the invalid fields in the first place but this code
>adopted the convention of validity bits and print what is there where valid or
>not. So maybe the things we should print are:
>Taking GMER as an example.

Hi Jonathan,

>
>Add the component id valid format to show_valid_flags() Then always print the
>flags for comp_id_pldm_flags() above whether or not they are valid. Finally print
>the two broken out fields Enity ID and Resource ID.
Will add component id valid format to show_valid_flags().
>
>Whether we also just print the raw versions is another question we need to
>figure out - in the formatted case it is duplication, but if not it includes more
>bytes.
Can do.

For the following component ID with PLDM entity and resource Id are valid
0x03 0x07 0xc5 0x08 0x9a 0x1a 0x0b 0xfc 0x0d 0x0e 0x2f 0x00 0x00 0x00  0x00  0x00

For example GMER trace printed as, 

"comp_id=PLDM_Entity_ID[5:0]  | Resource_ID[9:6] 07 c5 08 9a 1a 0b fc 0d 0e 2f validity_flags='CHANNEL|RANK|DEVICE|COMPONENT|0x10'"

cxl_general_media: memdev=mem0 host=0000:0f:00.0 serial=3 log=Fatal : time=246865829224 uuid=fbcd0a77-c260-417f-85a9-088b1621eba6 len=128 flags='0x1' handle=1 related_handle=0 maint_op_class=0 maint_op_sub_class=0 : dpa=7c0 dpa_flags='' descriptor='UNCORRECTABLE_EVENT|THRESHOLD_EVENT|POISON_LIST_OVERFLOW' type='TE State Violation' sub_type='Media Link Command Training Error' transaction_type='0xc0' channel=3 rank=33 device=5 comp_id=PLDM_Entity_ID[5:0]  | Resource_ID[9:6] 07 c5 08 9a 1a 0b fc 0d 0e 2f validity_flags='CHANNEL|RANK|DEVICE|COMPONENT|0x10' hpa=ffffffffffffffff cme_threshold_ev_flags='Corrected Memory Errors in Multiple Media Components|Exceeded Programmable Threshold' cme_count=78 region= region_uuid=00000000-0000-0000-0000-000000000000

Presently print comp_id_pldm_flags with [bitN: bitM] and then corresponding PLDM entity and/or resource ID based on the flags checking in the cxl_print_component_id().
 
Other cases,
"comp_id=PLDM_Entity_ID[5:0]  07 c5 08 9a 1a 0b validity_flags='CHANNEL|RANK|DEVICE|COMPONENT|0x10'" 
"comp_id= Resource_ID[9:6] fc 0d 0e 2f validity_flags='CHANNEL|RANK|DEVICE|COMPONENT|0x10'"

If need to modify, can print as you suggested,  
... validity_flags, raw comp_id, comp_id_pldm_flags, Enity ID, Resource ID ...
>
>Jonathan
>
Thanks,
Shiju

>
>
>>  /*
>>   * General Media Event Record - GMER
>>   * CXL rev 3.0 Section 8.2.9.2.1.1; Table 8-43