* [PATCH v2] cxl/events: Use a common struct for DRAM and General Media events
@ 2024-05-16 10:19 Fabio M. De Francesco
2024-05-16 16:33 ` Jonathan Cameron
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Fabio M. De Francesco @ 2024-05-16 10:19 UTC (permalink / raw
To: Davidlohr Bueso, Jonathan Cameron, Dave Jiang, Alison Schofield,
Vishal Verma, Ira Weiny, Dan Williams, linux-cxl, linux-kernel
Cc: Fabio M. De Francesco
cxl_event_common was a poor naming choice and caused confusion with the
existing Common Event Record.
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")
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(-)
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;
cxlr = cxl_dpa_to_region(cxlmd, dpa);
if (cxlr)
hpa = cxl_trace_hpa(cxlr, cxlmd, dpa);
if (event_type == CXL_CPER_EVENT_GEN_MEDIA)
trace_cxl_general_media(cxlmd, type, cxlr, hpa,
- &evt->gen_media);
+ &evt->media_general);
else if (event_type == CXL_CPER_EVENT_DRAM)
- trace_cxl_dram(cxlmd, type, cxlr, hpa, &evt->dram);
+ trace_cxl_dram(cxlmd, type, cxlr, hpa, &evt->media_dram);
}
}
EXPORT_SYMBOL_NS_GPL(cxl_event_trace_record, CXL);
diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h
index 07a0394b1d99..2c7293761bb2 100644
--- a/drivers/cxl/core/trace.h
+++ b/drivers/cxl/core/trace.h
@@ -316,7 +316,7 @@ TRACE_EVENT(cxl_generic_event,
TRACE_EVENT(cxl_general_media,
TP_PROTO(const struct cxl_memdev *cxlmd, enum cxl_event_log_type log,
- struct cxl_region *cxlr, u64 hpa, struct cxl_event_gen_media *rec),
+ struct cxl_region *cxlr, u64 hpa, struct cxl_event_media *rec),
TP_ARGS(cxlmd, log, cxlr, hpa, rec),
@@ -413,7 +413,7 @@ TRACE_EVENT(cxl_general_media,
TRACE_EVENT(cxl_dram,
TP_PROTO(const struct cxl_memdev *cxlmd, enum cxl_event_log_type log,
- struct cxl_region *cxlr, u64 hpa, struct cxl_event_dram *rec),
+ struct cxl_region *cxlr, u64 hpa, struct cxl_event_media *rec),
TP_ARGS(cxlmd, log, cxlr, hpa, rec),
diff --git a/include/linux/cxl-event.h b/include/linux/cxl-event.h
index 60b25020281f..e417556cc120 100644
--- a/include/linux/cxl-event.h
+++ b/include/linux/cxl-event.h
@@ -32,41 +32,38 @@ struct cxl_event_generic {
* CXL rev 3.0 Section 8.2.9.2.1.1; Table 8-43
*/
#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;
- u8 device[3];
- u8 component_id[CXL_EVENT_GEN_MED_COMP_ID_SIZE];
- u8 reserved[46];
-} __packed;
-
/*
* DRAM Event Record - DER
* CXL rev 3.0 section 8.2.9.2.1.2; Table 3-44
*/
#define CXL_EVENT_DER_CORRECTION_MASK_SIZE 0x20
-struct cxl_event_dram {
+struct cxl_event_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;
- u8 nibble_mask[3];
- u8 bank_group;
- u8 bank;
- u8 row[3];
- u8 column[2];
- u8 correction_mask[CXL_EVENT_DER_CORRECTION_MASK_SIZE];
- u8 reserved[0x17];
+ struct_group_tagged(cxl_event_media_hdr, media_hdr,
+ __le64 phys_addr;
+ u8 descriptor;
+ u8 type;
+ u8 transaction_type;
+ u8 validity_flags[2];
+ u8 channel;
+ u8 rank;
+ );
+ union {
+ struct_group(general,
+ u8 device[3];
+ u8 component_id[CXL_EVENT_GEN_MED_COMP_ID_SIZE];
+ u8 gen_reserved[46];
+ );
+ struct_group(dram,
+ u8 nibble_mask[3];
+ u8 bank_group;
+ u8 bank;
+ u8 row[3];
+ u8 column[2];
+ u8 correction_mask[CXL_EVENT_DER_CORRECTION_MASK_SIZE];
+ u8 dram_reserved[0x17];
+ );
+ };
} __packed;
/*
@@ -95,21 +92,12 @@ 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_media media_general;
+ struct cxl_event_media media_dram;
struct cxl_event_mem_module mem_module;
- struct cxl_event_common common;
+ struct cxl_event_media media_common;
} __packed;
/*
diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
index 6584443144de..0a8fd145c391 100644
--- a/tools/testing/cxl/test/mem.c
+++ b/tools/testing/cxl/test/mem.c
@@ -378,7 +378,7 @@ struct cxl_event_record_raw hardware_replace = {
struct cxl_test_gen_media {
uuid_t id;
- struct cxl_event_gen_media rec;
+ struct cxl_event_media rec;
} __packed;
struct cxl_test_gen_media gen_media = {
@@ -402,7 +402,7 @@ struct cxl_test_gen_media gen_media = {
struct cxl_test_dram {
uuid_t id;
- struct cxl_event_dram rec;
+ struct cxl_event_media rec;
} __packed;
struct cxl_test_dram dram = {
--
2.45.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] cxl/events: Use a common struct for DRAM and General Media events
2024-05-16 10:19 [PATCH v2] cxl/events: Use a common struct for DRAM and General Media events Fabio M. De Francesco
@ 2024-05-16 16:33 ` Jonathan Cameron
2024-05-17 10:08 ` Fabio M. De Francesco
2024-05-16 21:34 ` Ira Weiny
2024-05-17 0:43 ` Dan Williams
2 siblings, 1 reply; 6+ messages in thread
From: Jonathan Cameron @ 2024-05-16 16:33 UTC (permalink / raw
To: Fabio M. De Francesco
Cc: Davidlohr Bueso, Dave Jiang, Alison Schofield, Vishal Verma,
Ira Weiny, Dan Williams, linux-cxl, linux-kernel
On Thu, 16 May 2024 12:19:53 +0200
"Fabio M. De Francesco" <fabio.m.de.francesco@linux.intel.com> wrote:
> cxl_event_common was a poor naming choice and caused confusion with the
> existing Common Event Record.
>
> 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")
> Signed-off-by: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com>
> ---
Packing question inline.
>
> 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(-)
>
> 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;
> cxlr = cxl_dpa_to_region(cxlmd, dpa);
> if (cxlr)
> hpa = cxl_trace_hpa(cxlr, cxlmd, dpa);
>
> if (event_type == CXL_CPER_EVENT_GEN_MEDIA)
> trace_cxl_general_media(cxlmd, type, cxlr, hpa,
> - &evt->gen_media);
> + &evt->media_general);
> else if (event_type == CXL_CPER_EVENT_DRAM)
> - trace_cxl_dram(cxlmd, type, cxlr, hpa, &evt->dram);
> + trace_cxl_dram(cxlmd, type, cxlr, hpa, &evt->media_dram);
> }
> }
> EXPORT_SYMBOL_NS_GPL(cxl_event_trace_record, CXL);
> diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h
> index 07a0394b1d99..2c7293761bb2 100644
> --- a/drivers/cxl/core/trace.h
> +++ b/drivers/cxl/core/trace.h
> @@ -316,7 +316,7 @@ TRACE_EVENT(cxl_generic_event,
> TRACE_EVENT(cxl_general_media,
>
> TP_PROTO(const struct cxl_memdev *cxlmd, enum cxl_event_log_type log,
> - struct cxl_region *cxlr, u64 hpa, struct cxl_event_gen_media *rec),
> + struct cxl_region *cxlr, u64 hpa, struct cxl_event_media *rec),
>
> TP_ARGS(cxlmd, log, cxlr, hpa, rec),
>
> @@ -413,7 +413,7 @@ TRACE_EVENT(cxl_general_media,
> TRACE_EVENT(cxl_dram,
>
> TP_PROTO(const struct cxl_memdev *cxlmd, enum cxl_event_log_type log,
> - struct cxl_region *cxlr, u64 hpa, struct cxl_event_dram *rec),
> + struct cxl_region *cxlr, u64 hpa, struct cxl_event_media *rec),
>
> TP_ARGS(cxlmd, log, cxlr, hpa, rec),
>
> diff --git a/include/linux/cxl-event.h b/include/linux/cxl-event.h
> index 60b25020281f..e417556cc120 100644
> --- a/include/linux/cxl-event.h
> +++ b/include/linux/cxl-event.h
> @@ -32,41 +32,38 @@ struct cxl_event_generic {
> * CXL rev 3.0 Section 8.2.9.2.1.1; Table 8-43
> */
> #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;
> - u8 device[3];
> - u8 component_id[CXL_EVENT_GEN_MED_COMP_ID_SIZE];
> - u8 reserved[46];
> -} __packed;
> -
> /*
> * DRAM Event Record - DER
> * CXL rev 3.0 section 8.2.9.2.1.2; Table 3-44
> */
> #define CXL_EVENT_DER_CORRECTION_MASK_SIZE 0x20
> -struct cxl_event_dram {
> +struct cxl_event_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;
> - u8 nibble_mask[3];
> - u8 bank_group;
> - u8 bank;
> - u8 row[3];
> - u8 column[2];
> - u8 correction_mask[CXL_EVENT_DER_CORRECTION_MASK_SIZE];
> - u8 reserved[0x17];
> + struct_group_tagged(cxl_event_media_hdr, media_hdr,
> + __le64 phys_addr;
> + u8 descriptor;
> + u8 type;
> + u8 transaction_type;
> + u8 validity_flags[2];
> + u8 channel;
> + u8 rank;
> + );
Does the struct that is created end up __packed?
Also, why is tagged useful here?
> + union {
> + struct_group(general,
> + u8 device[3];
> + u8 component_id[CXL_EVENT_GEN_MED_COMP_ID_SIZE];
> + u8 gen_reserved[46];
> + );
> + struct_group(dram,
> + u8 nibble_mask[3];
> + u8 bank_group;
> + u8 bank;
> + u8 row[3];
> + u8 column[2];
> + u8 correction_mask[CXL_EVENT_DER_CORRECTION_MASK_SIZE];
> + u8 dram_reserved[0x17];
> + );
> + };
> } __packed;
>
> /*
> @@ -95,21 +92,12 @@ 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_media media_general;
> + struct cxl_event_media media_dram;
> struct cxl_event_mem_module mem_module;
> - struct cxl_event_common common;
> + struct cxl_event_media media_common;
> } __packed;
>
> /*
> diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
> index 6584443144de..0a8fd145c391 100644
> --- a/tools/testing/cxl/test/mem.c
> +++ b/tools/testing/cxl/test/mem.c
> @@ -378,7 +378,7 @@ struct cxl_event_record_raw hardware_replace = {
>
> struct cxl_test_gen_media {
> uuid_t id;
> - struct cxl_event_gen_media rec;
> + struct cxl_event_media rec;
> } __packed;
>
> struct cxl_test_gen_media gen_media = {
> @@ -402,7 +402,7 @@ struct cxl_test_gen_media gen_media = {
>
> struct cxl_test_dram {
> uuid_t id;
> - struct cxl_event_dram rec;
> + struct cxl_event_media rec;
> } __packed;
>
> struct cxl_test_dram dram = {
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] cxl/events: Use a common struct for DRAM and General Media events
2024-05-16 10:19 [PATCH v2] cxl/events: Use a common struct for DRAM and General Media events Fabio M. De Francesco
2024-05-16 16:33 ` Jonathan Cameron
@ 2024-05-16 21:34 ` Ira Weiny
2024-05-17 0:43 ` Dan Williams
2 siblings, 0 replies; 6+ messages in thread
From: Ira Weiny @ 2024-05-16 21:34 UTC (permalink / raw
To: Fabio M. De Francesco, Davidlohr Bueso, Jonathan Cameron,
Dave Jiang, Alison Schofield, Vishal Verma, Ira Weiny,
Dan Williams, linux-cxl, linux-kernel
Cc: Fabio M. De Francesco
Fabio M. De Francesco wrote:
> cxl_event_common was a poor naming choice and caused confusion with the
> existing Common Event Record.
>
> 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")
> 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(-)
Still not convinced this is worth the churn.
Ira
>
> 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;
> cxlr = cxl_dpa_to_region(cxlmd, dpa);
> if (cxlr)
> hpa = cxl_trace_hpa(cxlr, cxlmd, dpa);
>
> if (event_type == CXL_CPER_EVENT_GEN_MEDIA)
> trace_cxl_general_media(cxlmd, type, cxlr, hpa,
> - &evt->gen_media);
> + &evt->media_general);
> else if (event_type == CXL_CPER_EVENT_DRAM)
> - trace_cxl_dram(cxlmd, type, cxlr, hpa, &evt->dram);
> + trace_cxl_dram(cxlmd, type, cxlr, hpa, &evt->media_dram);
> }
> }
> EXPORT_SYMBOL_NS_GPL(cxl_event_trace_record, CXL);
> diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h
> index 07a0394b1d99..2c7293761bb2 100644
> --- a/drivers/cxl/core/trace.h
> +++ b/drivers/cxl/core/trace.h
> @@ -316,7 +316,7 @@ TRACE_EVENT(cxl_generic_event,
> TRACE_EVENT(cxl_general_media,
>
> TP_PROTO(const struct cxl_memdev *cxlmd, enum cxl_event_log_type log,
> - struct cxl_region *cxlr, u64 hpa, struct cxl_event_gen_media *rec),
> + struct cxl_region *cxlr, u64 hpa, struct cxl_event_media *rec),
>
> TP_ARGS(cxlmd, log, cxlr, hpa, rec),
>
> @@ -413,7 +413,7 @@ TRACE_EVENT(cxl_general_media,
> TRACE_EVENT(cxl_dram,
>
> TP_PROTO(const struct cxl_memdev *cxlmd, enum cxl_event_log_type log,
> - struct cxl_region *cxlr, u64 hpa, struct cxl_event_dram *rec),
> + struct cxl_region *cxlr, u64 hpa, struct cxl_event_media *rec),
>
> TP_ARGS(cxlmd, log, cxlr, hpa, rec),
>
> diff --git a/include/linux/cxl-event.h b/include/linux/cxl-event.h
> index 60b25020281f..e417556cc120 100644
> --- a/include/linux/cxl-event.h
> +++ b/include/linux/cxl-event.h
> @@ -32,41 +32,38 @@ struct cxl_event_generic {
> * CXL rev 3.0 Section 8.2.9.2.1.1; Table 8-43
> */
> #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;
> - u8 device[3];
> - u8 component_id[CXL_EVENT_GEN_MED_COMP_ID_SIZE];
> - u8 reserved[46];
> -} __packed;
> -
> /*
> * DRAM Event Record - DER
> * CXL rev 3.0 section 8.2.9.2.1.2; Table 3-44
> */
> #define CXL_EVENT_DER_CORRECTION_MASK_SIZE 0x20
> -struct cxl_event_dram {
> +struct cxl_event_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;
> - u8 nibble_mask[3];
> - u8 bank_group;
> - u8 bank;
> - u8 row[3];
> - u8 column[2];
> - u8 correction_mask[CXL_EVENT_DER_CORRECTION_MASK_SIZE];
> - u8 reserved[0x17];
> + struct_group_tagged(cxl_event_media_hdr, media_hdr,
> + __le64 phys_addr;
> + u8 descriptor;
> + u8 type;
> + u8 transaction_type;
> + u8 validity_flags[2];
> + u8 channel;
> + u8 rank;
> + );
> + union {
> + struct_group(general,
> + u8 device[3];
> + u8 component_id[CXL_EVENT_GEN_MED_COMP_ID_SIZE];
> + u8 gen_reserved[46];
> + );
> + struct_group(dram,
> + u8 nibble_mask[3];
> + u8 bank_group;
> + u8 bank;
> + u8 row[3];
> + u8 column[2];
> + u8 correction_mask[CXL_EVENT_DER_CORRECTION_MASK_SIZE];
> + u8 dram_reserved[0x17];
> + );
> + };
> } __packed;
>
> /*
> @@ -95,21 +92,12 @@ 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_media media_general;
> + struct cxl_event_media media_dram;
> struct cxl_event_mem_module mem_module;
> - struct cxl_event_common common;
> + struct cxl_event_media media_common;
> } __packed;
>
> /*
> diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
> index 6584443144de..0a8fd145c391 100644
> --- a/tools/testing/cxl/test/mem.c
> +++ b/tools/testing/cxl/test/mem.c
> @@ -378,7 +378,7 @@ struct cxl_event_record_raw hardware_replace = {
>
> struct cxl_test_gen_media {
> uuid_t id;
> - struct cxl_event_gen_media rec;
> + struct cxl_event_media rec;
> } __packed;
>
> struct cxl_test_gen_media gen_media = {
> @@ -402,7 +402,7 @@ struct cxl_test_gen_media gen_media = {
>
> struct cxl_test_dram {
> uuid_t id;
> - struct cxl_event_dram rec;
> + struct cxl_event_media rec;
> } __packed;
>
> struct cxl_test_dram dram = {
> --
> 2.45.0
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH v2] cxl/events: Use a common struct for DRAM and General Media events
2024-05-16 10:19 [PATCH v2] cxl/events: Use a common struct for DRAM and General Media events Fabio M. De Francesco
2024-05-16 16:33 ` Jonathan Cameron
2024-05-16 21:34 ` Ira Weiny
@ 2024-05-17 0:43 ` Dan Williams
2024-05-17 13:16 ` Fabio M. De Francesco
2 siblings, 1 reply; 6+ messages in thread
From: Dan Williams @ 2024-05-17 0:43 UTC (permalink / raw
To: Fabio M. De Francesco, Davidlohr Bueso, Jonathan Cameron,
Dave Jiang, Alison Schofield, Vishal Verma, Ira Weiny,
Dan Williams, linux-cxl, linux-kernel
Cc: Fabio M. De Francesco
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".
> 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?
> 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".
> cxlr = cxl_dpa_to_region(cxlmd, dpa);
> if (cxlr)
> hpa = cxl_trace_hpa(cxlr, cxlmd, dpa);
>
> if (event_type == CXL_CPER_EVENT_GEN_MEDIA)
> trace_cxl_general_media(cxlmd, type, cxlr, hpa,
> - &evt->gen_media);
> + &evt->media_general);
> else if (event_type == CXL_CPER_EVENT_DRAM)
> - trace_cxl_dram(cxlmd, type, cxlr, hpa, &evt->dram);
> + trace_cxl_dram(cxlmd, type, cxlr, hpa, &evt->media_dram);
> }
> }
> EXPORT_SYMBOL_NS_GPL(cxl_event_trace_record, CXL);
> diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h
> index 07a0394b1d99..2c7293761bb2 100644
> --- a/drivers/cxl/core/trace.h
> +++ b/drivers/cxl/core/trace.h
> @@ -316,7 +316,7 @@ TRACE_EVENT(cxl_generic_event,
> TRACE_EVENT(cxl_general_media,
>
> TP_PROTO(const struct cxl_memdev *cxlmd, enum cxl_event_log_type log,
> - struct cxl_region *cxlr, u64 hpa, struct cxl_event_gen_media *rec),
> + struct cxl_region *cxlr, u64 hpa, struct cxl_event_media *rec),
>
> TP_ARGS(cxlmd, log, cxlr, hpa, rec),
>
> @@ -413,7 +413,7 @@ TRACE_EVENT(cxl_general_media,
> TRACE_EVENT(cxl_dram,
>
> TP_PROTO(const struct cxl_memdev *cxlmd, enum cxl_event_log_type log,
> - struct cxl_region *cxlr, u64 hpa, struct cxl_event_dram *rec),
> + struct cxl_region *cxlr, u64 hpa, struct cxl_event_media *rec),
>
> TP_ARGS(cxlmd, log, cxlr, hpa, rec),
>
> diff --git a/include/linux/cxl-event.h b/include/linux/cxl-event.h
> index 60b25020281f..e417556cc120 100644
> --- a/include/linux/cxl-event.h
> +++ b/include/linux/cxl-event.h
> @@ -32,41 +32,38 @@ struct cxl_event_generic {
> * CXL rev 3.0 Section 8.2.9.2.1.1; Table 8-43
> */
> #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;
> - u8 device[3];
> - u8 component_id[CXL_EVENT_GEN_MED_COMP_ID_SIZE];
> - u8 reserved[46];
> -} __packed;
> -
> /*
> * DRAM Event Record - DER
> * CXL rev 3.0 section 8.2.9.2.1.2; Table 3-44
> */
> #define CXL_EVENT_DER_CORRECTION_MASK_SIZE 0x20
> -struct cxl_event_dram {
> +struct cxl_event_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;
> - u8 nibble_mask[3];
> - u8 bank_group;
> - u8 bank;
> - u8 row[3];
> - u8 column[2];
> - u8 correction_mask[CXL_EVENT_DER_CORRECTION_MASK_SIZE];
> - u8 reserved[0x17];
> + struct_group_tagged(cxl_event_media_hdr, media_hdr,
> + __le64 phys_addr;
> + u8 descriptor;
> + u8 type;
> + u8 transaction_type;
> + u8 validity_flags[2];
> + u8 channel;
> + u8 rank;
> + );
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? 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;
/*
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] cxl/events: Use a common struct for DRAM and General Media events
2024-05-16 16:33 ` Jonathan Cameron
@ 2024-05-17 10:08 ` Fabio M. De Francesco
0 siblings, 0 replies; 6+ messages in thread
From: Fabio M. De Francesco @ 2024-05-17 10:08 UTC (permalink / raw
To: Jonathan Cameron
Cc: Davidlohr Bueso, Dave Jiang, Alison Schofield, Vishal Verma,
Ira Weiny, Dan Williams, linux-cxl, linux-kernel
Jonathan,
Thanks for your comments.
On Thursday, May 16, 2024 6:33:19 PM GMT+2 Jonathan Cameron wrote:
> On Thu, 16 May 2024 12:19:53 +0200
>
> "Fabio M. De Francesco" <fabio.m.de.francesco@linux.intel.com> wrote:
> > cxl_event_common was a poor naming choice and caused confusion with the
> > existing Common Event Record.
> >
> > 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") Signed-off-by: Fabio M. De Francesco
> > <fabio.m.de.francesco@linux.intel.com> ---
>
> Packing question inline.
>
> > [...]
> >
> > -struct cxl_event_dram {
> > +struct cxl_event_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;
> > - u8 nibble_mask[3];
> > - u8 bank_group;
> > - u8 bank;
> > - u8 row[3];
> > - u8 column[2];
> > - u8 correction_mask[CXL_EVENT_DER_CORRECTION_MASK_SIZE];
> > - u8 reserved[0x17];
> > + struct_group_tagged(cxl_event_media_hdr, media_hdr,
> > + __le64 phys_addr;
> > + u8 descriptor;
> > + u8 type;
> > + u8 transaction_type;
> > + u8 validity_flags[2];
> > + u8 channel;
> > + u8 rank;
> > + );
>
> Does the struct that is created end up __packed?
No, I should have noticed it.
> Also, why is tagged useful here?
It is not useful. I'll rework it not tagged.
Again thanks,
Fabio
> > + union {
> > + struct_group(general,
> > + u8 device[3];
> > + u8
component_id[CXL_EVENT_GEN_MED_COMP_ID_SIZE];
> > + u8 gen_reserved[46];
> > + );
> > + struct_group(dram,
> > + u8 nibble_mask[3];
> > + u8 bank_group;
> > + u8 bank;
> > + u8 row[3];
> > + u8 column[2];
> > + u8
correction_mask[CXL_EVENT_DER_CORRECTION_MASK_SIZE];
> > + u8 dram_reserved[0x17];
> > + );
> > + };
> >
> > } __packed;
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] cxl/events: Use a common struct for DRAM and General Media events
2024-05-17 0:43 ` Dan Williams
@ 2024-05-17 13:16 ` Fabio M. De Francesco
0 siblings, 0 replies; 6+ messages in thread
From: Fabio M. De Francesco @ 2024-05-17 13:16 UTC (permalink / raw
To: Dan Williams, Dan Williams
Cc: Davidlohr Bueso, Jonathan Cameron, Dave Jiang, Alison Schofield,
Vishal Verma, Ira Weiny, linux-cxl, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-05-17 13:16 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-16 10:19 [PATCH v2] cxl/events: Use a common struct for DRAM and General Media events Fabio M. De Francesco
2024-05-16 16:33 ` Jonathan Cameron
2024-05-17 10:08 ` Fabio M. De Francesco
2024-05-16 21:34 ` Ira Weiny
2024-05-17 0:43 ` Dan Williams
2024-05-17 13:16 ` Fabio M. De Francesco
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.