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 <[email protected]>
Signed-off-by: Fabio M. De Francesco <[email protected]>
---
- 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 v2 -
https://lore.kernel.org/linux-cxl/[email protected]/
drivers/cxl/core/mbox.c | 2 +-
drivers/cxl/core/trace.h | 32 ++++++++++++++---------------
include/linux/cxl-event.h | 40 +++++++++++++-----------------------
tools/testing/cxl/test/mem.c | 30 +++++++++++++--------------
4 files changed, 46 insertions(+), 58 deletions(-)
diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index 2626f3fff201..a08f050cc1ca 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -875,7 +875,7 @@ 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_hdr.phys_addr) & CXL_DPA_MASK;
cxlr = cxl_dpa_to_region(cxlmd, dpa);
if (cxlr)
hpa = cxl_trace_hpa(cxlr, cxlmd, dpa);
diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h
index 07a0394b1d99..5a76f94accf4 100644
--- a/drivers/cxl/core/trace.h
+++ b/drivers/cxl/core/trace.h
@@ -340,23 +340,23 @@ TRACE_EVENT(cxl_general_media,
),
TP_fast_assign(
- CXL_EVT_TP_fast_assign(cxlmd, log, rec->hdr);
+ CXL_EVT_TP_fast_assign(cxlmd, log, rec->media_hdr.hdr);
__entry->hdr_uuid = CXL_EVENT_GEN_MEDIA_UUID;
/* General Media */
- __entry->dpa = le64_to_cpu(rec->phys_addr);
+ __entry->dpa = le64_to_cpu(rec->media_hdr.phys_addr);
__entry->dpa_flags = __entry->dpa & CXL_DPA_FLAGS_MASK;
/* Mask after flags have been parsed */
__entry->dpa &= CXL_DPA_MASK;
- __entry->descriptor = rec->descriptor;
- __entry->type = rec->type;
- __entry->transaction_type = rec->transaction_type;
- __entry->channel = rec->channel;
- __entry->rank = rec->rank;
+ __entry->descriptor = rec->media_hdr.descriptor;
+ __entry->type = rec->media_hdr.type;
+ __entry->transaction_type = rec->media_hdr.transaction_type;
+ __entry->channel = rec->media_hdr.channel;
+ __entry->rank = rec->media_hdr.rank;
__entry->device = get_unaligned_le24(rec->device);
memcpy(__entry->comp_id, &rec->component_id,
CXL_EVENT_GEN_MED_COMP_ID_SIZE);
- __entry->validity_flags = get_unaligned_le16(&rec->validity_flags);
+ __entry->validity_flags = get_unaligned_le16(&rec->media_hdr.validity_flags);
__entry->hpa = hpa;
if (cxlr) {
__assign_str(region_name, dev_name(&cxlr->dev));
@@ -440,19 +440,19 @@ TRACE_EVENT(cxl_dram,
),
TP_fast_assign(
- CXL_EVT_TP_fast_assign(cxlmd, log, rec->hdr);
+ CXL_EVT_TP_fast_assign(cxlmd, log, rec->media_hdr.hdr);
__entry->hdr_uuid = CXL_EVENT_DRAM_UUID;
/* DRAM */
- __entry->dpa = le64_to_cpu(rec->phys_addr);
+ __entry->dpa = le64_to_cpu(rec->media_hdr.phys_addr);
__entry->dpa_flags = __entry->dpa & CXL_DPA_FLAGS_MASK;
__entry->dpa &= CXL_DPA_MASK;
- __entry->descriptor = rec->descriptor;
- __entry->type = rec->type;
- __entry->transaction_type = rec->transaction_type;
- __entry->validity_flags = get_unaligned_le16(rec->validity_flags);
- __entry->channel = rec->channel;
- __entry->rank = rec->rank;
+ __entry->descriptor = rec->media_hdr.descriptor;
+ __entry->type = rec->media_hdr.type;
+ __entry->transaction_type = rec->media_hdr.transaction_type;
+ __entry->validity_flags = get_unaligned_le16(rec->media_hdr.validity_flags);
+ __entry->channel = rec->media_hdr.channel;
+ __entry->rank = rec->media_hdr.rank;
__entry->nibble_mask = get_unaligned_le24(rec->nibble_mask);
__entry->bank_group = rec->bank_group;
__entry->bank = rec->bank;
diff --git a/include/linux/cxl-event.h b/include/linux/cxl-event.h
index 60b25020281f..6562663a036d 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];
+ 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,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_mem_module mem_module;
- struct cxl_event_common common;
+ struct cxl_event_media_hdr media_hdr;
} __packed;
/*
diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
index 6584443144de..63b71541d399 100644
--- a/tools/testing/cxl/test/mem.c
+++ b/tools/testing/cxl/test/mem.c
@@ -384,19 +384,19 @@ struct cxl_test_gen_media {
struct cxl_test_gen_media gen_media = {
.id = CXL_EVENT_GEN_MEDIA_UUID,
.rec = {
- .hdr = {
+ .media_hdr.hdr = {
.length = sizeof(struct cxl_test_gen_media),
.flags[0] = CXL_EVENT_RECORD_FLAG_PERMANENT,
/* .handle = Set dynamically */
.related_handle = cpu_to_le16(0),
},
- .phys_addr = cpu_to_le64(0x2000),
- .descriptor = CXL_GMER_EVT_DESC_UNCORECTABLE_EVENT,
- .type = CXL_GMER_MEM_EVT_TYPE_DATA_PATH_ERROR,
- .transaction_type = CXL_GMER_TRANS_HOST_WRITE,
+ .media_hdr.phys_addr = cpu_to_le64(0x2000),
+ .media_hdr.descriptor = CXL_GMER_EVT_DESC_UNCORECTABLE_EVENT,
+ .media_hdr.type = CXL_GMER_MEM_EVT_TYPE_DATA_PATH_ERROR,
+ .media_hdr.transaction_type = CXL_GMER_TRANS_HOST_WRITE,
/* .validity_flags = <set below> */
- .channel = 1,
- .rank = 30
+ .media_hdr.channel = 1,
+ .media_hdr.rank = 30
},
};
@@ -408,18 +408,18 @@ struct cxl_test_dram {
struct cxl_test_dram dram = {
.id = CXL_EVENT_DRAM_UUID,
.rec = {
- .hdr = {
+ .media_hdr.hdr = {
.length = sizeof(struct cxl_test_dram),
.flags[0] = CXL_EVENT_RECORD_FLAG_PERF_DEGRADED,
/* .handle = Set dynamically */
.related_handle = cpu_to_le16(0),
},
- .phys_addr = cpu_to_le64(0x8000),
- .descriptor = CXL_GMER_EVT_DESC_THRESHOLD_EVENT,
- .type = CXL_GMER_MEM_EVT_TYPE_INV_ADDR,
- .transaction_type = CXL_GMER_TRANS_INTERNAL_MEDIA_SCRUB,
+ .media_hdr.phys_addr = cpu_to_le64(0x8000),
+ .media_hdr.descriptor = CXL_GMER_EVT_DESC_THRESHOLD_EVENT,
+ .media_hdr.type = CXL_GMER_MEM_EVT_TYPE_INV_ADDR,
+ .media_hdr.transaction_type = CXL_GMER_TRANS_INTERNAL_MEDIA_SCRUB,
/* .validity_flags = <set below> */
- .channel = 1,
+ .media_hdr.channel = 1,
.bank_group = 5,
.bank = 2,
.column = {0xDE, 0xAD},
@@ -473,11 +473,11 @@ static int mock_set_timestamp(struct cxl_dev_state *cxlds,
static void cxl_mock_add_event_logs(struct mock_event_store *mes)
{
put_unaligned_le16(CXL_GMER_VALID_CHANNEL | CXL_GMER_VALID_RANK,
- &gen_media.rec.validity_flags);
+ &gen_media.rec.media_hdr.validity_flags);
put_unaligned_le16(CXL_DER_VALID_CHANNEL | CXL_DER_VALID_BANK_GROUP |
CXL_DER_VALID_BANK | CXL_DER_VALID_COLUMN,
- &dram.rec.validity_flags);
+ &dram.rec.media_hdr.validity_flags);
mes_add_event(mes, CXL_EVENT_TYPE_INFO, &maint_needed);
mes_add_event(mes, CXL_EVENT_TYPE_INFO,
--
2.45.0
On Saturday, May 18, 2024 1:26:21 PM GMT+2 Fabio M. De Francesco 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 <[email protected]>
> Signed-off-by: Fabio M. De Francesco <[email protected]>
> ---
>
> [...]
> -
> 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;
> + struct cxl_event_media_hdr media_hdr;
> } __packed;
Today I was thinking about a comment from Ira. He didn't like the addition of
an event that is not in the specs.[0] (Notice that the other issues have been
already addressed).
I dislike the addition of an artificial event for the very same reasons Ira
expressed.
This additional event could be easily removed by something simple like the
following diff.
Fabio
[0] https://lore.kernel.org/linux-cxl/[email protected]/
diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index a08f050cc1ca..05de8836adea 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -875,7 +875,13 @@ 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->media_hdr.phys_addr) & CXL_DPA_MASK;
+ if (event_type == CXL_CPER_EVENT_GEN_MEDIA)
+ dpa = le64_to_cpu(evt->gen_media.media_hdr.phys_addr)
+ & CXL_DPA_MASK;
+ else if (event_type == CXL_CPER_EVENT_GEN_MEDIA)
+ dpa = le64_to_cpu(evt->dram.media_hdr.phys_addr)
+ & CXL_DPA_MASK;
+
cxlr = cxl_dpa_to_region(cxlmd, dpa);
if (cxlr)
hpa = cxl_trace_hpa(cxlr, cxlmd, dpa);
diff --git a/include/linux/cxl-event.h b/include/linux/cxl-event.h
index 6562663a036d..f0a5be131e6a 100644
--- a/include/linux/cxl-event.h
+++ b/include/linux/cxl-event.h
@@ -97,7 +97,6 @@ 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_media_hdr media_hdr;
} __packed;
/*
On Mon, May 20, 2024 at 07:55:17PM +0200, Fabio M. De Francesco wrote:
> On Saturday, May 18, 2024 1:26:21 PM GMT+2 Fabio M. De Francesco 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 <[email protected]>
> > Signed-off-by: Fabio M. De Francesco <[email protected]>
> > ---
> >
> > [...]
> > -
> > 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;
> > + struct cxl_event_media_hdr media_hdr;
> > } __packed;
>
> Today I was thinking about a comment from Ira. He didn't like the addition of
> an event that is not in the specs.[0] (Notice that the other issues have been
> already addressed).
>
> I dislike the addition of an artificial event for the very same reasons Ira
> expressed.
Agree it should not appear like an artifical event. It should appear
as a special member of the union. A union's purpose is to allow access
like this. Perhaps it's a naming thing.
How about s/cxl_event_media_hdr/cxl_media_hdr and add a comment:
- struct cxl_event_common common;
+ struct cxl_media_hdr media_hdr; /* dram & gen_media event header */
>
> This additional event could be easily removed by something simple like the
> following diff.
Some typos below and also you'll need to look at the larger chunk of
code and reconsider the if/else flow. Perhaps revert to an earlier
version of the event patchset.
-- Alison
>
> Fabio
>
> [0] https://lore.kernel.org/linux-cxl/[email protected]/
>
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index a08f050cc1ca..05de8836adea 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -875,7 +875,13 @@ 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->media_hdr.phys_addr) & CXL_DPA_MASK;
> + if (event_type == CXL_CPER_EVENT_GEN_MEDIA)
> + dpa = le64_to_cpu(evt->gen_media.media_hdr.phys_addr)
> + & CXL_DPA_MASK;
> + else if (event_type == CXL_CPER_EVENT_GEN_MEDIA)
> + dpa = le64_to_cpu(evt->dram.media_hdr.phys_addr)
> + & CXL_DPA_MASK;
> +
> cxlr = cxl_dpa_to_region(cxlmd, dpa);
> if (cxlr)
> hpa = cxl_trace_hpa(cxlr, cxlmd, dpa);
> diff --git a/include/linux/cxl-event.h b/include/linux/cxl-event.h
> index 6562663a036d..f0a5be131e6a 100644
> --- a/include/linux/cxl-event.h
> +++ b/include/linux/cxl-event.h
> @@ -97,7 +97,6 @@ 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_media_hdr media_hdr;
> } __packed;
>
> /*
>
>
>
Fabio M. De Francesco 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 <[email protected]>
> Signed-off-by: Fabio M. De Francesco <[email protected]>
> ---
>
> - 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 v2 -
>
> https://lore.kernel.org/linux-cxl/[email protected]/
>
>
> drivers/cxl/core/mbox.c | 2 +-
> drivers/cxl/core/trace.h | 32 ++++++++++++++---------------
> include/linux/cxl-event.h | 40 +++++++++++++-----------------------
> tools/testing/cxl/test/mem.c | 30 +++++++++++++--------------
> 4 files changed, 46 insertions(+), 58 deletions(-)
>
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index 2626f3fff201..a08f050cc1ca 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -875,7 +875,7 @@ 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_hdr.phys_addr) & CXL_DPA_MASK;
> cxlr = cxl_dpa_to_region(cxlmd, dpa);
> if (cxlr)
> hpa = cxl_trace_hpa(cxlr, cxlmd, dpa);
> diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h
> index 07a0394b1d99..5a76f94accf4 100644
> --- a/drivers/cxl/core/trace.h
> +++ b/drivers/cxl/core/trace.h
> @@ -340,23 +340,23 @@ TRACE_EVENT(cxl_general_media,
[..]
Changes look good.
> @@ -440,19 +440,19 @@ TRACE_EVENT(cxl_dram,
[..]
Looks good.
[..]
> diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
> index 6584443144de..63b71541d399 100644
> --- a/tools/testing/cxl/test/mem.c
> +++ b/tools/testing/cxl/test/mem.c
> @@ -384,19 +384,19 @@ struct cxl_test_gen_media {
> struct cxl_test_gen_media gen_media = {
> .id = CXL_EVENT_GEN_MEDIA_UUID,
> .rec = {
> - .hdr = {
> + .media_hdr.hdr = {
> .length = sizeof(struct cxl_test_gen_media),
> .flags[0] = CXL_EVENT_RECORD_FLAG_PERMANENT,
> /* .handle = Set dynamically */
> .related_handle = cpu_to_le16(0),
> },
> - .phys_addr = cpu_to_le64(0x2000),
> - .descriptor = CXL_GMER_EVT_DESC_UNCORECTABLE_EVENT,
> - .type = CXL_GMER_MEM_EVT_TYPE_DATA_PATH_ERROR,
> - .transaction_type = CXL_GMER_TRANS_HOST_WRITE,
> + .media_hdr.phys_addr = cpu_to_le64(0x2000),
> + .media_hdr.descriptor = CXL_GMER_EVT_DESC_UNCORECTABLE_EVENT,
> + .media_hdr.type = CXL_GMER_MEM_EVT_TYPE_DATA_PATH_ERROR,
> + .media_hdr.transaction_type = CXL_GMER_TRANS_HOST_WRITE,
> /* .validity_flags = <set below> */
> - .channel = 1,
> - .rank = 30
> + .media_hdr.channel = 1,
> + .media_hdr.rank = 30
This looks awkward, I would have expected a conversion like:
@@ -384,19 +384,21 @@ struct cxl_test_gen_media {
struct cxl_test_gen_media gen_media = {
.id = CXL_EVENT_GEN_MEDIA_UUID,
.rec = {
- .hdr = {
- .length = sizeof(struct cxl_test_gen_media),
- .flags[0] = CXL_EVENT_RECORD_FLAG_PERMANENT,
- /* .handle = Set dynamically */
- .related_handle = cpu_to_le16(0),
+ .media_hdr = {
+ .hdr = {
+ .length = sizeof(struct cxl_test_gen_media),
+ .flags[0] = CXL_EVENT_RECORD_FLAG_PERMANENT,
+ /* .handle = Set dynamically */
+ .related_handle = cpu_to_le16(0),
+ },
+ .phys_addr = cpu_to_le64(0x2000),
+ .descriptor = CXL_GMER_EVT_DESC_UNCORECTABLE_EVENT,
+ .type = CXL_GMER_MEM_EVT_TYPE_DATA_PATH_ERROR,
+ .transaction_type = CXL_GMER_TRANS_HOST_WRITE,
+ /* .validity_flags = <set below> */
+ .channel = 1,
+ .rank = 30
},
- .phys_addr = cpu_to_le64(0x2000),
- .descriptor = CXL_GMER_EVT_DESC_UNCORECTABLE_EVENT,
- .type = CXL_GMER_MEM_EVT_TYPE_DATA_PATH_ERROR,
- .transaction_type = CXL_GMER_TRANS_HOST_WRITE,
- /* .validity_flags = <set below> */
- .channel = 1,
- .rank = 30
},
};
> },
> };
>
> @@ -408,18 +408,18 @@ struct cxl_test_dram {
> struct cxl_test_dram dram = {
> .id = CXL_EVENT_DRAM_UUID,
> .rec = {
> - .hdr = {
> + .media_hdr.hdr = {
Same comment as above, just scope the declarations with out extra
de-references.
Fabio M. De Francesco wrote:
> On Monday, May 20, 2024 7:55:17 PM GMT+2 Fabio M. De Francesco wrote:
> > On Saturday, May 18, 2024 1:26:21 PM GMT+2 Fabio M. De Francesco wrote:
> > [...]
> I suspect that I didn't clarify that the diff above is proposing an additional
> little change to this patch (for v4) and that I wanted to collect comments
> before applying and respinning.
>
> To be clearer, that diff is meant only to show that cxl_event_media_hdr can be
> removed from union cxl_event at no cost while still be used for the common
> fields in the definitions of cxl_event_dram and cxl_event_gen_media.
I agree with Alison, this is what a union is for, and more generally,
runtime code should never be added to make up for deficiencies in the data
structure.
On Monday, May 20, 2024 7:55:17 PM GMT+2 Fabio M. De Francesco wrote:
> On Saturday, May 18, 2024 1:26:21 PM GMT+2 Fabio M. De Francesco wrote:
> [...]
>
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index a08f050cc1ca..05de8836adea 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -875,7 +875,13 @@ 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->media_hdr.phys_addr) & CXL_DPA_MASK;
> + if (event_type == CXL_CPER_EVENT_GEN_MEDIA)
> + dpa = le64_to_cpu(evt-
>gen_media.media_hdr.phys_addr)
> + & CXL_DPA_MASK;
> + else if (event_type == CXL_CPER_EVENT_GEN_MEDIA)
> + dpa = le64_to_cpu(evt->dram.media_hdr.phys_addr)
> + & CXL_DPA_MASK;
> +
> cxlr = cxl_dpa_to_region(cxlmd, dpa);
> if (cxlr)
> hpa = cxl_trace_hpa(cxlr, cxlmd, dpa);
> diff --git a/include/linux/cxl-event.h b/include/linux/cxl-event.h
> index 6562663a036d..f0a5be131e6a 100644
> --- a/include/linux/cxl-event.h
> +++ b/include/linux/cxl-event.h
> @@ -97,7 +97,6 @@ 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_media_hdr media_hdr;
> } __packed;
>
> /*
>
I suspect that I didn't clarify that the diff above is proposing an additional
little change to this patch (for v4) and that I wanted to collect comments
before applying and respinning.
To be clearer, that diff is meant only to show that cxl_event_media_hdr can be
removed from union cxl_event at no cost while still be used for the common
fields in the definitions of cxl_event_dram and cxl_event_gen_media.
Fabio