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

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

On Wednesday, June 5, 2024 1:30:00 PM GMT+2 Jonathan Cameron wrote:
> On Sun,  2 Jun 2024 21:12:25 +0200
> "Fabio M. De Francesco" <fabio.m.de.francesco@linux.intel.com> 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>
> > Reviewed-by: Alison Schofield <alison.schofield@intel.com>
> > Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> > Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> > Signed-off-by: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com>
> > ---
> > 
> > - Changes for v6 -
> > 
> > 	- Add "Reviewed-by" tags
> Hi Fabio,

Hi Jonathan,
 
> Adding tags doesn't need a new version. b4 or however Dave is picking these
> up will gather them up from the v5 posting.

I just read  what you replied in the v5 thread and know that now it makes more 
sense.

> My one nervousness about this is the impression it perhaps gives that the 
contents
> of validity flags has the same meaning in both records.
> 
> Maybe a comment to that effect next to it's definition makes sense?

Yes, sure. 
I'll add it.

> I think the alternative of not including that or the remaining elements in
> your common structure would be worse.
> 
> Jonathan
> 

Thank you,

Fabio

> 
> > 
> > - Changes for v5 -
> > 
> > 	- Rebase on v6.10-rc1
> > 
> > - Changes for v4 -
> > 
> >         - Initialise cxl_test_dram and cxl_test_gen_media without 
> >           unnecessary extra de-references (Dan)
> >         - Add a comment for media_hdr in union cxl_event (Alison)
> > 
> > - 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 v4 -
> > 
> > https://lore.kernel.org/linux-cxl/20240521140750.26035-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    | 41 ++++++++++-----------------
> >  tools/testing/cxl/test/mem.c | 54 +++++++++++++++++++-----------------
> >  4 files changed, 61 insertions(+), 68 deletions(-)
> 
> 
> 
> > diff --git a/include/linux/cxl-event.h b/include/linux/cxl-event.h
> > index 60b25020281f..1119d0bbb091 100644
> > --- a/include/linux/cxl-event.h
> > +++ b/include/linux/cxl-event.h
> > @@ -21,6 +21,17 @@ struct cxl_event_record_hdr {
> >  	u8 reserved[15];
> >  } __packed;
> >  
> > +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];
> 
> Perhaps a comment to say that validity_flags meaning after bit 2
> varies across the different records?
> 
> > +	u8 channel;
> > +	u8 rank;
> > +} __packed;
> > +
> >  #define CXL_EVENT_RECORD_DATA_LENGTH 0x50
> >  struct cxl_event_generic {
> >  	struct cxl_event_record_hdr hdr;
> > @@ -33,14 +44,7 @@ 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 media_hdr;
> >  	u8 device[3];
> >  	u8 component_id[CXL_EVENT_GEN_MED_COMP_ID_SIZE];
> >  	u8 reserved[46];
> > @@ -52,14 +56,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 media_hdr;
> >  	u8 nibble_mask[3];
> >  	u8 bank_group;
> >  	u8 bank;
> > @@ -95,21 +92,13 @@ struct cxl_event_mem_module {
> >  	u8 reserved[0x3d];
> >  } __packed;
> >  
> > -/*
> > - * General Media or DRAM Event Common Fields
> > - * - provides common access to phys_addr
> > - */
> > -struct cxl_event_common {
> > -	struct cxl_event_record_hdr hdr;
> > -	__le64 phys_addr;
> > -} __packed;
> > -
> >  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;
> > +	/* dram & gen_media event header */
> > +	struct cxl_event_media_hdr media_hdr;
> >  } __packed;
> 
> 
> 
> 
>