Messages in this thread | ![/](/images/icornerl.gif) | | From | "Fabio M. De Francesco" <> | Subject | Re: [PATCH v2] cxl/events: Use a common struct for DRAM and General Media events | Date | Fri, 17 May 2024 15:16:09 +0200 |
| |
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
| ![\](/images/icornerr.gif) |