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

["Fabio M. De Francesco" <fabio.m.de.francesco@linux.intel.com>]

On Friday, May 17, 2024 2:43:33 AM GMT+2 Dan Williams wrote:
> Fabio M. De Francesco wrote:
> > cxl_event_common was a poor naming choice and caused confusion with the
> > existing Common Event Record.
> 
> I would say "unfortunate" rather than "poor".
>

Agreed.
 
> > Use cxl_event_media as a common structure to record information about DRAM
> > and General Media events because it simplifies handling the two events.
> > 
> > Suggested-by: Dan Williams <dan.j.williams@intel.com>
> > Fixes: 6aec00139d3a ("cxl/core: Add region info to cxl_general_media and
> > cxl_dram events")
> What is the fix? There is no user visible behavior changes that results from
> this cleanup, right?

Right.
 
> > Signed-off-by: Fabio M. De Francesco
> > <fabio.m.de.francesco@linux.intel.com>
> > ---
> > 
> > Changes for v2:
> > 	- Extend the commit message (Alison);
> > 	- Add a "Fixes" tag (Alison, thanks).
> > 	
> >  drivers/cxl/core/mbox.c      |  6 ++--
> >  drivers/cxl/core/trace.h     |  4 +--
> >  include/linux/cxl-event.h    | 70 +++++++++++++++---------------------
> >  tools/testing/cxl/test/mem.c |  4 +--
> >  4 files changed, 36 insertions(+), 48 deletions(-)
> 
> Oh, nice, net reduction in code I was not expecting.
> 
> > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> > index 2626f3fff201..ad4d7b0f7f4d 100644
> > --- a/drivers/cxl/core/mbox.c
> > +++ b/drivers/cxl/core/mbox.c
> > @@ -875,16 +875,16 @@ 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_common.phys_addr) & 
CXL_DPA_MASK;
> 
> I was hoping at the end of this to get rid of the word "common" to avoid any
> more "common event record confusion".

Right, still an unfortunate naming choice. 

> > [...]
> 
> Oh nice you went for the full 'struct_group_tagged' proposal. However, given
> that Jonathan is already asking questions about "__packed", lets do the
> simpler change, something like this, what do you think? 

I think that these simpler changes are better. 

> This also preserves
> type-safety.
> 
> diff --git a/include/linux/cxl-event.h b/include/linux/cxl-event.h
> index 60b25020281f..f48b59943977 100644
> --- a/include/linux/cxl-event.h
> +++ b/include/linux/cxl-event.h
> @@ -33,14 +33,16 @@ struct cxl_event_generic {
>   */
>  #define CXL_EVENT_GEN_MED_COMP_ID_SIZE 0x10
>  struct cxl_event_gen_media {
> -       struct cxl_event_record_hdr hdr;
> -       __le64 phys_addr;
> -       u8 descriptor;
> -       u8 type;
> -       u8 transaction_type;
> -       u8 validity_flags[2];
> -       u8 channel;
> -       u8 rank;
> +       struct cxl_event_media_hdr {
> +               struct cxl_event_record_hdr hdr;
> +               __le64 phys_addr;
> +               u8 descriptor;
> +               u8 type;
> +               u8 transaction_type;
> +               u8 validity_flags[2];
> +               u8 channel;
> +               u8 rank;
> +       } hdr;
>         u8 device[3];
>         u8 component_id[CXL_EVENT_GEN_MED_COMP_ID_SIZE];
>         u8 reserved[46];
> @@ -52,14 +54,7 @@ struct cxl_event_gen_media {
>   */
>  #define CXL_EVENT_DER_CORRECTION_MASK_SIZE     0x20
>  struct cxl_event_dram {
> -       struct cxl_event_record_hdr hdr;
> -       __le64 phys_addr;
> -       u8 descriptor;
> -       u8 type;
> -       u8 transaction_type;
> -       u8 validity_flags[2];
> -       u8 channel;
> -       u8 rank;
> +       struct cxl_event_media_hdr hdr;
>         u8 nibble_mask[3];
>         u8 bank_group;
>         u8 bank;
> @@ -109,7 +104,7 @@ 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_common common;
> +       struct cxl_event_media_hdr media_hdr;
>  } __packed;
> 
>  /*

I'll make v3 according to the suggestion showed above.

Thanks,

Fabio