1. Patchew
  2. linux
  3. View series

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

Fabio M. De Francesco posted 1 patch 1 year, 8 months ago
Re: [PATCH v3] cxl/events: Use a common struct for DRAM and General Media events
Posted by Fabio M. De Francesco 1 year, 8 months ago 
On Saturday, May 18, 2024 1:26:21 PM GMT+2 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>
> ---
> 
> [...]
> -
>  union cxl_event {
>  	struct cxl_event_generic generic;
>  	struct cxl_event_gen_media gen_media;
>  	struct cxl_event_dram dram;
>  	struct cxl_event_mem_module mem_module;
> -	struct cxl_event_common common;
> +	struct cxl_event_media_hdr media_hdr;
>  } __packed;

Today I was thinking about a comment from Ira. He didn't like the addition of 
an event that is not in the specs.[0] (Notice that the other issues have been 
already addressed). 

I dislike the addition of an artificial event for the very same reasons Ira 
expressed.

This additional event could be easily removed by something simple like the 
following diff.

Fabio

[0] https://lore.kernel.org/linux-cxl/66467b8b47170_8c79294b3@iweiny-mobl.notmuch/

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index a08f050cc1ca..05de8836adea 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -875,7 +875,13 @@ 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->media_hdr.phys_addr) & CXL_DPA_MASK;
+               if (event_type == CXL_CPER_EVENT_GEN_MEDIA)
+                       dpa = le64_to_cpu(evt->gen_media.media_hdr.phys_addr)
+                             & CXL_DPA_MASK;
+               else if (event_type == CXL_CPER_EVENT_GEN_MEDIA)
+                       dpa = le64_to_cpu(evt->dram.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/include/linux/cxl-event.h b/include/linux/cxl-event.h
index 6562663a036d..f0a5be131e6a 100644
--- a/include/linux/cxl-event.h
+++ b/include/linux/cxl-event.h
@@ -97,7 +97,6 @@ union cxl_event {
        struct cxl_event_gen_media gen_media;
        struct cxl_event_dram dram;
        struct cxl_event_mem_module mem_module;
-       struct cxl_event_media_hdr media_hdr;
 } __packed;
 
 /*
Re: [PATCH v3] cxl/events: Use a common struct for DRAM and General Media events
Posted by Fabio M. De Francesco 1 year, 8 months ago 
On Monday, May 20, 2024 7:55:17 PM GMT+2 Fabio M. De Francesco wrote:
> On Saturday, May 18, 2024 1:26:21 PM GMT+2 Fabio M. De Francesco wrote:
> [...]
>
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index a08f050cc1ca..05de8836adea 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -875,7 +875,13 @@ 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->media_hdr.phys_addr) & CXL_DPA_MASK;
> +               if (event_type == CXL_CPER_EVENT_GEN_MEDIA)
> +                       dpa = le64_to_cpu(evt-
>gen_media.media_hdr.phys_addr)
> +                             & CXL_DPA_MASK;
> +               else if (event_type == CXL_CPER_EVENT_GEN_MEDIA)
> +                       dpa = le64_to_cpu(evt->dram.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/include/linux/cxl-event.h b/include/linux/cxl-event.h
> index 6562663a036d..f0a5be131e6a 100644
> --- a/include/linux/cxl-event.h
> +++ b/include/linux/cxl-event.h
> @@ -97,7 +97,6 @@ union cxl_event {
>         struct cxl_event_gen_media gen_media;
>         struct cxl_event_dram dram;
>         struct cxl_event_mem_module mem_module;
> -       struct cxl_event_media_hdr media_hdr;
>  } __packed;
>  
>  /*
> 

I suspect that I didn't clarify that the diff above is proposing an additional 
little change to this patch (for v4) and that I wanted to collect comments 
before applying and respinning.

To be clearer, that diff is meant only to show that cxl_event_media_hdr can be 
removed from union cxl_event at no cost while still be used for the common 
fields in the definitions of cxl_event_dram and cxl_event_gen_media.

Fabio
Re: [PATCH v3] cxl/events: Use a common struct for DRAM and General Media events
Posted by Dan Williams 1 year, 8 months ago 
Fabio M. De Francesco wrote:
> On Monday, May 20, 2024 7:55:17 PM GMT+2 Fabio M. De Francesco wrote:
> > On Saturday, May 18, 2024 1:26:21 PM GMT+2 Fabio M. De Francesco wrote:
> > [...]
> I suspect that I didn't clarify that the diff above is proposing an additional 
> little change to this patch (for v4) and that I wanted to collect comments 
> before applying and respinning.
> 
> To be clearer, that diff is meant only to show that cxl_event_media_hdr can be 
> removed from union cxl_event at no cost while still be used for the common 
> fields in the definitions of cxl_event_dram and cxl_event_gen_media.

I agree with Alison, this is what a union is for, and more generally,
runtime code should never be added to make up for deficiencies in the data
structure.
Re: [PATCH v3] cxl/events: Use a common struct for DRAM and General Media events
Posted by Alison Schofield 1 year, 8 months ago 
On Mon, May 20, 2024 at 07:55:17PM +0200, Fabio M. De Francesco wrote:
> On Saturday, May 18, 2024 1:26:21 PM GMT+2 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>
> > ---
> > 
> > [...]
> > -
> >  union cxl_event {
> >  	struct cxl_event_generic generic;
> >  	struct cxl_event_gen_media gen_media;
> >  	struct cxl_event_dram dram;
> >  	struct cxl_event_mem_module mem_module;
> > -	struct cxl_event_common common;
> > +	struct cxl_event_media_hdr media_hdr;
> >  } __packed;
> 
> Today I was thinking about a comment from Ira. He didn't like the addition of 
> an event that is not in the specs.[0] (Notice that the other issues have been 
> already addressed). 
> 
> I dislike the addition of an artificial event for the very same reasons Ira 
> expressed.

Agree it should not appear like an artifical event. It should appear
as a special member of the union. A union's purpose is to allow access
like this. Perhaps it's a naming thing.

How about s/cxl_event_media_hdr/cxl_media_hdr and add a comment:

-	struct cxl_event_common common;
+	struct cxl_media_hdr media_hdr; /* dram & gen_media event header */

> 
> This additional event could be easily removed by something simple like the 
> following diff.

Some typos below and also you'll need to look at the larger chunk of
code and reconsider the if/else flow. Perhaps revert to an earlier
version of the event patchset.

-- Alison

> 
> Fabio
> 
> [0] https://lore.kernel.org/linux-cxl/66467b8b47170_8c79294b3@iweiny-mobl.notmuch/
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index a08f050cc1ca..05de8836adea 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -875,7 +875,13 @@ 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->media_hdr.phys_addr) & CXL_DPA_MASK;
> +               if (event_type == CXL_CPER_EVENT_GEN_MEDIA)
> +                       dpa = le64_to_cpu(evt->gen_media.media_hdr.phys_addr)
> +                             & CXL_DPA_MASK;
> +               else if (event_type == CXL_CPER_EVENT_GEN_MEDIA)
> +                       dpa = le64_to_cpu(evt->dram.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/include/linux/cxl-event.h b/include/linux/cxl-event.h
> index 6562663a036d..f0a5be131e6a 100644
> --- a/include/linux/cxl-event.h
> +++ b/include/linux/cxl-event.h
> @@ -97,7 +97,6 @@ union cxl_event {
>         struct cxl_event_gen_media gen_media;
>         struct cxl_event_dram dram;
>         struct cxl_event_mem_module mem_module;
> -       struct cxl_event_media_hdr media_hdr;
>  } __packed;
>  
>  /*
> 
> 
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.