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

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

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.

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.

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.

Jonathan



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