lkml.org 
[lkml]   [2024]   [May]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH v2] cxl/events: Use a common struct for DRAM and General Media events
Date
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




\
 
 \ /
  Last update: 2024-05-27 18:30    [W:0.053 / U:0.796 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site