Re: [PATCH v3] cxl/events: Use a common struct for DRAM and General Media events

[Dan Williams <dan.j.williams@intel.com>]

Fabio M. De Francesco wrote:
> cxl_event_common was an unfortunate naming choice and caused confusion with
> the existing Common Event Record. Furthermore, its fields didn't map all
> the common information between DRAM and General Media Events.
> 
> Remove cxl_event_common and introduce cxl_event_media_hdr to record common
> information between DRAM and General Media events.
> 
> cxl_event_media_hdr, which is embedded in both cxl_event_gen_media and
> cxl_event_dram, leverages the commonalities between the two events to
> simplify their respective handling.
> 
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com>
> ---
> 
> - Changes for v3 -
> 
> 	- Rework the layout of cxl_event_dram and cxl_event_gen_media
> 	  to make a simpler change (Dan)
> 	- Remove a "Fixes" tag (Dan)
> 	- Don't use unnecessary struct_group[_tagged] (Jonathan, Ira)
> 	- Rewrite end extend the commit message
> 
> - Link to v2 -
> 
> 	https://lore.kernel.org/linux-cxl/20240516102116.3512377-1-fabio.m.de.francesco@linux.intel.com/
> 
> 
>  drivers/cxl/core/mbox.c      |  2 +-
>  drivers/cxl/core/trace.h     | 32 ++++++++++++++---------------
>  include/linux/cxl-event.h    | 40 +++++++++++++-----------------------
>  tools/testing/cxl/test/mem.c | 30 +++++++++++++--------------
>  4 files changed, 46 insertions(+), 58 deletions(-)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index 2626f3fff201..a08f050cc1ca 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -875,7 +875,7 @@ void cxl_event_trace_record(const struct cxl_memdev *cxlmd,
>  		guard(rwsem_read)(&cxl_region_rwsem);
>  		guard(rwsem_read)(&cxl_dpa_rwsem);
>  
> -		dpa = le64_to_cpu(evt->common.phys_addr) & CXL_DPA_MASK;
> +		dpa = le64_to_cpu(evt->media_hdr.phys_addr) & CXL_DPA_MASK;
>  		cxlr = cxl_dpa_to_region(cxlmd, dpa);
>  		if (cxlr)
>  			hpa = cxl_trace_hpa(cxlr, cxlmd, dpa);
> diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h
> index 07a0394b1d99..5a76f94accf4 100644
> --- a/drivers/cxl/core/trace.h
> +++ b/drivers/cxl/core/trace.h
> @@ -340,23 +340,23 @@ TRACE_EVENT(cxl_general_media,
[..]

Changes look good.

> @@ -440,19 +440,19 @@ TRACE_EVENT(cxl_dram,
[..]

Looks good.

[..]
> diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
> index 6584443144de..63b71541d399 100644
> --- a/tools/testing/cxl/test/mem.c
> +++ b/tools/testing/cxl/test/mem.c
> @@ -384,19 +384,19 @@ struct cxl_test_gen_media {
>  struct cxl_test_gen_media gen_media = {
>  	.id = CXL_EVENT_GEN_MEDIA_UUID,
>  	.rec = {
> -		.hdr = {
> +		.media_hdr.hdr = {
>  			.length = sizeof(struct cxl_test_gen_media),
>  			.flags[0] = CXL_EVENT_RECORD_FLAG_PERMANENT,
>  			/* .handle = Set dynamically */
>  			.related_handle = cpu_to_le16(0),
>  		},
> -		.phys_addr = cpu_to_le64(0x2000),
> -		.descriptor = CXL_GMER_EVT_DESC_UNCORECTABLE_EVENT,
> -		.type = CXL_GMER_MEM_EVT_TYPE_DATA_PATH_ERROR,
> -		.transaction_type = CXL_GMER_TRANS_HOST_WRITE,
> +		.media_hdr.phys_addr = cpu_to_le64(0x2000),
> +		.media_hdr.descriptor = CXL_GMER_EVT_DESC_UNCORECTABLE_EVENT,
> +		.media_hdr.type = CXL_GMER_MEM_EVT_TYPE_DATA_PATH_ERROR,
> +		.media_hdr.transaction_type = CXL_GMER_TRANS_HOST_WRITE,
>  		/* .validity_flags = <set below> */
> -		.channel = 1,
> -		.rank = 30
> +		.media_hdr.channel = 1,
> +		.media_hdr.rank = 30

This looks awkward, I would have expected a conversion like:

@@ -384,19 +384,21 @@ struct cxl_test_gen_media {
 struct cxl_test_gen_media gen_media = {
        .id = CXL_EVENT_GEN_MEDIA_UUID,
        .rec = {
-               .hdr = {
-                       .length = sizeof(struct cxl_test_gen_media),
-                       .flags[0] = CXL_EVENT_RECORD_FLAG_PERMANENT,
-                       /* .handle = Set dynamically */
-                       .related_handle = cpu_to_le16(0),
+               .media_hdr = {
+                       .hdr = {
+                               .length = sizeof(struct cxl_test_gen_media),
+                               .flags[0] = CXL_EVENT_RECORD_FLAG_PERMANENT,
+                               /* .handle = Set dynamically */
+                               .related_handle = cpu_to_le16(0),
+                       },
+                       .phys_addr = cpu_to_le64(0x2000),
+                       .descriptor = CXL_GMER_EVT_DESC_UNCORECTABLE_EVENT,
+                       .type = CXL_GMER_MEM_EVT_TYPE_DATA_PATH_ERROR,
+                       .transaction_type = CXL_GMER_TRANS_HOST_WRITE,
+                       /* .validity_flags = <set below> */
+                       .channel = 1,
+                       .rank = 30
                },
-               .phys_addr = cpu_to_le64(0x2000),
-               .descriptor = CXL_GMER_EVT_DESC_UNCORECTABLE_EVENT,
-               .type = CXL_GMER_MEM_EVT_TYPE_DATA_PATH_ERROR,
-               .transaction_type = CXL_GMER_TRANS_HOST_WRITE,
-               /* .validity_flags = <set below> */
-               .channel = 1,
-               .rank = 30
        },
 };



>  	},
>  };
>  
> @@ -408,18 +408,18 @@ struct cxl_test_dram {
>  struct cxl_test_dram dram = {
>  	.id = CXL_EVENT_DRAM_UUID,
>  	.rec = {
> -		.hdr = {
> +		.media_hdr.hdr = {

Same comment as above, just scope the declarations with out extra
de-references.