[PATCH v5 0/2] Incorporate DRAM address in EDAC messages

[PATCH v5 0/2] Incorporate DRAM address in EDAC messages

[Yazen Ghannam]

Hi all,

This revision addresses a comment from Sashiko in patch 2. The decision
to print the DRAM address will be independent of other decoding results.

Thanks,
Yazen

Cc: Avadhut Naik <avanaik92@gmail.com>

Link:
https://lore.kernel.org/all/20260316125324.396889-1-yazen.ghannam@amd.com/

Changes in v5:
 - Address a comment from Sashiko in patch 2.

Changes in v4:
 - Drop a "handler available" check in patch 1.
 - Reword commit message in patch 2.
 - Reformat string in patch 2.

Avadhut Naik (2):
  RAS/AMD/ATL: Translate UMC normalized address to DRAM address using
    PRM
  EDAC/amd64: Include DRAM address in output

 drivers/edac/amd64_edac.c      | 27 ++++++++++++++++++++++++++-
 drivers/edac/amd64_edac.h      |  1 +
 drivers/ras/amd/atl/core.c     |  3 ++-
 drivers/ras/amd/atl/internal.h | 11 ++++++++++-
 drivers/ras/amd/atl/prm.c      | 32 ++++++++++++++++++++++++++++----
 drivers/ras/amd/atl/system.c   |  3 +++
 drivers/ras/amd/atl/umc.c      |  9 +++++++++
 drivers/ras/ras.c              | 18 ++++++++++++++++--
 include/linux/ras.h            | 19 ++++++++++++++++++-
 9 files changed, 113 insertions(+), 10 deletions(-)

base-commit: 4b049cd5e5194c8e58cd978d2dc9d3bcf67c84f6
-- 
2.54.0

[PATCH v5 1/2] RAS/AMD/ATL: Translate UMC normalized address to DRAM address using PRM

[Yazen Ghannam]

From: Avadhut Naik <avadhut.naik@amd.com>

Modern AMD SOCs provide UEFI PRM module that implements various address
translation PRM handlers.[1] These handlers can be invoked by the OS or
hypervisor at runtime to perform address translations.

On AMD's Zen-based SOCs, Unified Memory Controller (UMC) relative
"normalized" address is reported through MCA_ADDR of UMC SMCA bank type
on occurrence of a DRAM ECC error. This address must be converted into
system physical address and DRAM address to export additional information
about the error.

Add support to convert normalized address into DRAM address through the
appropriate PRM handler. Instead of logging the translated DRAM address
locally, register the translating function when the Address Translation
library is initialized. Modules like amd64_edac can then invoke the PRM
handler to add the DRAM address to their error records. Additionally, it
can also be exported through the RAS tracepont.

[1] https://bugzilla.kernel.org/show_bug.cgi?id=220577

[Yazen: Remove 'handler available' check]

Signed-off-by: Avadhut Naik <avadhut.naik@amd.com>
Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---
 drivers/ras/amd/atl/core.c     |  3 ++-
 drivers/ras/amd/atl/internal.h | 11 ++++++++++-
 drivers/ras/amd/atl/prm.c      | 32 ++++++++++++++++++++++++++++----
 drivers/ras/amd/atl/system.c   |  3 +++
 drivers/ras/amd/atl/umc.c      |  9 +++++++++
 drivers/ras/ras.c              | 18 ++++++++++++++++--
 include/linux/ras.h            | 19 ++++++++++++++++++-
 7 files changed, 86 insertions(+), 9 deletions(-)

diff --git a/drivers/ras/amd/atl/core.c b/drivers/ras/amd/atl/core.c
index 0f7cd6dab0b0..3d622e6a6cc5 100644
--- a/drivers/ras/amd/atl/core.c
+++ b/drivers/ras/amd/atl/core.c
@@ -210,7 +210,8 @@ static int __init amd_atl_init(void)
 
 	/* Increment this module's recount so that it can't be easily unloaded. */
 	__module_get(THIS_MODULE);
-	amd_atl_register_decoder(convert_umc_mca_addr_to_sys_addr);
+	amd_atl_register_decoder(convert_umc_mca_addr_to_sys_addr,
+				 convert_umc_mca_addr_to_dram_addr);
 
 	pr_info("AMD Address Translation Library initialized\n");
 	return 0;
diff --git a/drivers/ras/amd/atl/internal.h b/drivers/ras/amd/atl/internal.h
index 82a56d9c2be1..b6ca5fef6ec7 100644
--- a/drivers/ras/amd/atl/internal.h
+++ b/drivers/ras/amd/atl/internal.h
@@ -280,23 +280,32 @@ int dehash_address(struct addr_ctx *ctx);
 
 unsigned long norm_to_sys_addr(u8 socket_id, u8 die_id, u8 coh_st_inst_id, unsigned long addr);
 unsigned long convert_umc_mca_addr_to_sys_addr(struct atl_err *err);
+int convert_umc_mca_addr_to_dram_addr(struct atl_err *err, struct atl_dram_addr *dram_addr);
 
 u64 add_base_and_hole(struct addr_ctx *ctx, u64 addr);
 u64 remove_base_and_hole(struct addr_ctx *ctx, u64 addr);
 
 /* GUIDs for PRM handlers */
 extern const guid_t norm_to_sys_guid;
+extern const guid_t norm_to_dram_guid;
 
 #ifdef CONFIG_AMD_ATL_PRM
 unsigned long prm_umc_norm_to_sys_addr(u8 socket_id, u64 umc_bank_inst_id, unsigned long addr);
+int prm_umc_norm_to_dram_addr(u8 socket_id, u64 bank_id,
+			      unsigned long addr, struct atl_dram_addr *dram_addr);
 #else
 static inline unsigned long prm_umc_norm_to_sys_addr(u8 socket_id, u64 umc_bank_inst_id,
 						     unsigned long addr)
 {
        return -ENODEV;
 }
-#endif
 
+static inline int prm_umc_norm_to_dram_addr(u8 socket_id, u64 bank_id,
+					    unsigned long addr, struct atl_dram_addr *dram_addr)
+{
+	return -ENODEV;
+}
+#endif
 /*
  * Make a gap in @data that is @num_bits long starting at @bit_num.
  * e.g. data		= 11111111'b
diff --git a/drivers/ras/amd/atl/prm.c b/drivers/ras/amd/atl/prm.c
index 0f9bfa96e16a..c69158f66639 100644
--- a/drivers/ras/amd/atl/prm.c
+++ b/drivers/ras/amd/atl/prm.c
@@ -19,10 +19,11 @@
 #include <linux/prmt.h>
 
 /*
- * PRM parameter buffer - normalized to system physical address, as described
- * in the "PRM Parameter Buffer" section of the AMD ACPI Porting Guide.
+ * PRM parameter buffer - normalized to system physical address and normalized
+ * to DRAM address, as described in the "PRM Parameter Buffer" section of the
+ * AMD ACPI Porting Guide.
  */
-struct norm_to_sys_param_buf {
+struct prm_parameter_buffer {
 	u64 norm_addr;
 	u8 socket;
 	u64 bank_id;
@@ -31,7 +32,7 @@ struct norm_to_sys_param_buf {
 
 unsigned long prm_umc_norm_to_sys_addr(u8 socket_id, u64 bank_id, unsigned long addr)
 {
-	struct norm_to_sys_param_buf p_buf;
+	struct prm_parameter_buffer p_buf;
 	unsigned long ret_addr;
 	int ret;
 
@@ -51,3 +52,26 @@ unsigned long prm_umc_norm_to_sys_addr(u8 socket_id, u64 bank_id, unsigned long
 
 	return ret;
 }
+
+int prm_umc_norm_to_dram_addr(u8 socket_id, u64 bank_id,
+			      unsigned long addr, struct atl_dram_addr *dram_addr)
+{
+	struct prm_parameter_buffer p_buf;
+	int ret;
+
+	p_buf.norm_addr	= addr;
+	p_buf.socket	= socket_id;
+	p_buf.bank_id	= bank_id;
+	p_buf.out_buf	= dram_addr;
+
+	ret = acpi_call_prm_handler(norm_to_dram_guid, &p_buf);
+	if (!ret)
+		return ret;
+
+	if (ret == -ENODEV)
+		pr_debug("PRM module/handler not available.\n");
+	else
+		pr_notice_once("PRM DRAM Address Translation failed.\n");
+
+	return ret;
+}
diff --git a/drivers/ras/amd/atl/system.c b/drivers/ras/amd/atl/system.c
index 812a30e21d3a..33a04f3e7da8 100644
--- a/drivers/ras/amd/atl/system.c
+++ b/drivers/ras/amd/atl/system.c
@@ -17,6 +17,9 @@
 const guid_t norm_to_sys_guid = GUID_INIT(0xE7180659, 0xA65D, 0x451D,
 					  0x92, 0xCD, 0x2B, 0x56, 0xF1,
 					  0x2B, 0xEB, 0xA6);
+const guid_t norm_to_dram_guid = GUID_INIT(0x7626C6AE, 0xF973, 0x429C,
+					  0xA9, 0x1C, 0x10, 0x7D, 0x7B,
+					  0xE2, 0x98, 0xB0);
 
 int determine_node_id(struct addr_ctx *ctx, u8 socket_id, u8 die_id)
 {
diff --git a/drivers/ras/amd/atl/umc.c b/drivers/ras/amd/atl/umc.c
index befc616d5e8a..b48c0fe97bf2 100644
--- a/drivers/ras/amd/atl/umc.c
+++ b/drivers/ras/amd/atl/umc.c
@@ -416,3 +416,12 @@ unsigned long convert_umc_mca_addr_to_sys_addr(struct atl_err *err)
 
 	return norm_to_sys_addr(socket_id, die_id, coh_st_inst_id, addr);
 }
+
+int convert_umc_mca_addr_to_dram_addr(struct atl_err *err, struct atl_dram_addr *dram_addr)
+{
+	u8 socket_id = topology_physical_package_id(err->cpu);
+	unsigned long addr = get_addr(err->addr);
+	u64 bank_id = err->ipid;
+
+	return prm_umc_norm_to_dram_addr(socket_id, bank_id, addr, dram_addr);
+}
diff --git a/drivers/ras/ras.c b/drivers/ras/ras.c
index 03df3db62334..582aa2b93e81 100644
--- a/drivers/ras/ras.c
+++ b/drivers/ras/ras.c
@@ -19,15 +19,20 @@
  */
 static unsigned long (*amd_atl_umc_na_to_spa)(struct atl_err *err);
 
-void amd_atl_register_decoder(unsigned long (*f)(struct atl_err *))
+static int (*amd_atl_umc_na_to_dram_addr)(struct atl_err *err, struct atl_dram_addr *dram_addr);
+
+void amd_atl_register_decoder(unsigned long (*f1)(struct atl_err *),
+			      int (*f2)(struct atl_err *, struct atl_dram_addr *))
 {
-	amd_atl_umc_na_to_spa = f;
+	amd_atl_umc_na_to_spa = f1;
+	amd_atl_umc_na_to_dram_addr = f2;
 }
 EXPORT_SYMBOL_GPL(amd_atl_register_decoder);
 
 void amd_atl_unregister_decoder(void)
 {
 	amd_atl_umc_na_to_spa = NULL;
+	amd_atl_umc_na_to_dram_addr = NULL;
 }
 EXPORT_SYMBOL_GPL(amd_atl_unregister_decoder);
 
@@ -39,6 +44,15 @@ unsigned long amd_convert_umc_mca_addr_to_sys_addr(struct atl_err *err)
 	return amd_atl_umc_na_to_spa(err);
 }
 EXPORT_SYMBOL_GPL(amd_convert_umc_mca_addr_to_sys_addr);
+
+int amd_convert_umc_mca_addr_to_dram_addr(struct atl_err *err, struct atl_dram_addr *dram_addr)
+{
+	if (!amd_atl_umc_na_to_dram_addr)
+		return -EINVAL;
+
+	return amd_atl_umc_na_to_dram_addr(err, dram_addr);
+}
+EXPORT_SYMBOL_GPL(amd_convert_umc_mca_addr_to_dram_addr);
 #endif /* CONFIG_AMD_ATL */
 
 #define CREATE_TRACE_POINTS
diff --git a/include/linux/ras.h b/include/linux/ras.h
index 468941bfe855..1e82a94b0db0 100644
--- a/include/linux/ras.h
+++ b/include/linux/ras.h
@@ -41,15 +41,32 @@ struct atl_err {
 	u32 cpu;
 };
 
+struct atl_dram_addr {
+	u8 chip_select;
+	u8 bank_group;
+	u8 bank_addr;
+	u32 row_addr;
+	u16 col_addr;
+	u8 rank_mul;
+	u8 sub_ch;
+} __packed;
+
 #if IS_ENABLED(CONFIG_AMD_ATL)
-void amd_atl_register_decoder(unsigned long (*f)(struct atl_err *));
+void amd_atl_register_decoder(unsigned long (*f1)(struct atl_err *),
+			      int (*f2)(struct atl_err *, struct atl_dram_addr *));
 void amd_atl_unregister_decoder(void);
 void amd_retire_dram_row(struct atl_err *err);
 unsigned long amd_convert_umc_mca_addr_to_sys_addr(struct atl_err *err);
+int amd_convert_umc_mca_addr_to_dram_addr(struct atl_err *err, struct atl_dram_addr *dram_addr);
 #else
 static inline void amd_retire_dram_row(struct atl_err *err) { }
 static inline unsigned long
 amd_convert_umc_mca_addr_to_sys_addr(struct atl_err *err) { return -EINVAL; }
+static inline int amd_convert_umc_mca_addr_to_dram_addr(struct atl_err *err,
+							struct atl_dram_addr *dram_addr)
+{
+	return -EINVAL;
+}
 #endif /* CONFIG_AMD_ATL */
 
 #if defined(CONFIG_ARM) || defined(CONFIG_ARM64)
-- 
2.54.0

[PATCH v5 2/2] EDAC/amd64: Include DRAM address in output

[Yazen Ghannam]

From: Avadhut Naik <avadhut.naik@amd.com>

The DRAM address of an error is used by tooling to find failure
patterns. This information can be used for general analysis off system.
And it can be used on system to take action like offline a page affected
by a bad row.

Other EDAC modules (GHES and SKX) provide this information in their
output. The AMD64 EDAC module was not able to provide this information,
because system-specific translation is needed.

Recent AMD systems provide a PRM handler for DRAM address translation.

Use this PRM handler to get the DRAM address of an error. Include this
in the EDAC "other_detail" field.

[Yazen: Reword commit message and reformat other_detail string]

Signed-off-by: Avadhut Naik <avadhut.naik@amd.com>
Co-developed-by: Yazen Ghannam <yazen.ghannam@amd.com>
Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---
 drivers/edac/amd64_edac.c | 27 ++++++++++++++++++++++++++-
 drivers/edac/amd64_edac.h |  1 +
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index c6aa69dbd9fb..4f1c06b68907 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -2704,11 +2704,16 @@ static int get_channel_from_ecc_syndrome(struct mem_ctl_info *mci, u16 syndrome)
 	return map_err_sym_to_channel(err_sym, pvt->ecc_sym_sz);
 }
 
+#define MSG_SIZE 512
+
 static void __log_ecc_error(struct mem_ctl_info *mci, struct err_info *err,
 			    u8 ecc_type)
 {
 	enum hw_event_mc_err_type err_type;
 	const char *string;
+	char s[MSG_SIZE];
+
+	memset(s, 0, sizeof(s));
 
 	if (ecc_type == 2)
 		err_type = HW_EVENT_ERR_CORRECTED;
@@ -2721,6 +2726,21 @@ static void __log_ecc_error(struct mem_ctl_info *mci, struct err_info *err,
 		return;
 	}
 
+	if (err->dram_addr) {
+		struct atl_dram_addr *da = err->dram_addr;
+		char *p = s, *end = p + sizeof(s);
+
+		/* Include a version prefix in case the format needs to change later. */
+		p += scnprintf(p, end - p, " [AMDv1]");
+		p += scnprintf(p, end - p, " %s:0x%x", "ChipSelect",	da->chip_select);
+		p += scnprintf(p, end - p, " %s:0x%x", "Row",		da->row_addr);
+		p += scnprintf(p, end - p, " %s:0x%x", "Column",	da->col_addr);
+		p += scnprintf(p, end - p, " %s:0x%x", "Bank",		da->bank_addr);
+		p += scnprintf(p, end - p, " %s:0x%x", "BankGroup",	da->bank_group);
+		p += scnprintf(p, end - p, " %s:0x%x", "RankMul",	da->rank_mul);
+		p += scnprintf(p, end - p, " %s:0x%x", "SubChannel",	da->sub_ch);
+	}
+
 	switch (err->err_code) {
 	case DECODE_OK:
 		string = "";
@@ -2748,7 +2768,7 @@ static void __log_ecc_error(struct mem_ctl_info *mci, struct err_info *err,
 	edac_mc_handle_error(err_type, mci, 1,
 			     err->page, err->offset, err->syndrome,
 			     err->csrow, err->channel, -1,
-			     string, "");
+			     string, s);
 }
 
 static inline void decode_bus_error(int node_id, struct mce *m)
@@ -2808,6 +2828,7 @@ static void umc_get_err_info(struct mce *m, struct err_info *err)
 static void decode_umc_error(int node_id, struct mce *m)
 {
 	u8 ecc_type = (m->status >> 45) & 0x3;
+	struct atl_dram_addr dram_addr;
 	struct mem_ctl_info *mci;
 	unsigned long sys_addr;
 	struct amd64_pvt *pvt;
@@ -2822,6 +2843,7 @@ static void decode_umc_error(int node_id, struct mce *m)
 
 	pvt = mci->pvt_info;
 
+	memset(&dram_addr, 0, sizeof(dram_addr));
 	memset(&err, 0, sizeof(err));
 
 	if (m->status & MCI_STATUS_DEFERRED)
@@ -2853,6 +2875,9 @@ static void decode_umc_error(int node_id, struct mce *m)
 		goto log_error;
 	}
 
+	if (!amd_convert_umc_mca_addr_to_dram_addr(&a_err, &dram_addr))
+		err.dram_addr = &dram_addr;
+
 	error_address_to_page_and_offset(sys_addr, &err);
 
 log_error:
diff --git a/drivers/edac/amd64_edac.h b/drivers/edac/amd64_edac.h
index 1757c1b99fc8..e0ad1c0fc1c3 100644
--- a/drivers/edac/amd64_edac.h
+++ b/drivers/edac/amd64_edac.h
@@ -400,6 +400,7 @@ struct err_info {
 	u16 syndrome;
 	u32 page;
 	u32 offset;
+	struct atl_dram_addr *dram_addr;
 };
 
 static inline u32 get_umc_base(u8 channel)
-- 
2.54.0

Re: [PATCH v5 0/2] Incorporate DRAM address in EDAC messages

[Borislav Petkov]

On Mon, May 18, 2026 at 12:07:14PM -0400, Yazen Ghannam wrote:
> Hi all,
> 
> This revision addresses a comment from Sashiko in patch 2. The decision
> to print the DRAM address will be independent of other decoding results.

https://sashiko.dev/#/patchset/20260518160716.171578-1-yazen.ghannam%40amd.com

Meh, gemini still finds stuff. Some of it is meh, the fake_inject thing
probably needs fixing tho as I can see the concurrent f*ckup happening...

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v5 0/2] Incorporate DRAM address in EDAC messages

[Yazen Ghannam]

On Wed, May 20, 2026 at 05:57:40PM -0700, Borislav Petkov wrote:
> On Mon, May 18, 2026 at 12:07:14PM -0400, Yazen Ghannam wrote:
> > Hi all,
> > 
> > This revision addresses a comment from Sashiko in patch 2. The decision
> > to print the DRAM address will be independent of other decoding results.
> 
> https://sashiko.dev/#/patchset/20260518160716.171578-1-yazen.ghannam%40amd.com
> 
> Meh, gemini still finds stuff. Some of it is meh, the fake_inject thing
> probably needs fixing tho as I can see the concurrent f*ckup happening...
> 

The fake_inject thing looks like it could happen with all EDAC modules.

But it seems unlikely because CONFIG_EDAC_DEBUG needs to be enabled to
enable fake_inject. Plus a user would need to use it at the same time as
a real DRAM ECC error occurs.

We could add a mutex to the mci struct and hold it during
edac_mc_handle_error().

What do you think?

Thanks,
Yazen

Re: [PATCH v5 0/2] Incorporate DRAM address in EDAC messages

[Borislav Petkov]

On Mon, Jun 01, 2026 at 02:41:00PM -0400, Yazen Ghannam wrote:
> The fake_inject thing looks like it could happen with all EDAC modules.
> 
> But it seems unlikely because CONFIG_EDAC_DEBUG needs to be enabled to
> enable fake_inject. Plus a user would need to use it at the same time as
> a real DRAM ECC error occurs.
> 
> We could add a mutex to the mci struct and hold it during
> edac_mc_handle_error().
> 
> What do you think?

TBH, I'm wondering if this thing is even used and whether we should simply
kill it...

Meh:

452a6bf955ee ("edac: Add debufs nodes to allow doing fake error inject")

it probably is useful.

How about instead of passing silly ptrs around, we copy the string to a buffer
local to edac_mc_handle_error() and avoid stupid locking? In a pre-patch
perhaps...

This whole thing is itching me to axe it off but meh...

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v5 0/2] Incorporate DRAM address in EDAC messages

[Yazen Ghannam]

On Mon, Jun 01, 2026 at 02:05:43PM -0700, Borislav Petkov wrote:
> On Mon, Jun 01, 2026 at 02:41:00PM -0400, Yazen Ghannam wrote:
> > The fake_inject thing looks like it could happen with all EDAC modules.
> > 
> > But it seems unlikely because CONFIG_EDAC_DEBUG needs to be enabled to
> > enable fake_inject. Plus a user would need to use it at the same time as
> > a real DRAM ECC error occurs.
> > 
> > We could add a mutex to the mci struct and hold it during
> > edac_mc_handle_error().
> > 
> > What do you think?
> 
> TBH, I'm wondering if this thing is even used and whether we should simply
> kill it...
> 
> Meh:
> 
> 452a6bf955ee ("edac: Add debufs nodes to allow doing fake error inject")
> 
> it probably is useful.
> 
> How about instead of passing silly ptrs around, we copy the string to a buffer
> local to edac_mc_handle_error() and avoid stupid locking? In a pre-patch
> perhaps...

I think the issue is having concurrent updates to mci->error_desc. EDAC
assumes a single owner for the MCIs. The fake_inject interface basically
acts as a second module/owner.

> 
> This whole thing is itching me to axe it off but meh...
> 

Right, I don't think it's a big deal in practice. IMO, this is a
non-issue.

Thanks,
Yazen

Re: [PATCH v5 0/2] Incorporate DRAM address in EDAC messages

[Borislav Petkov]

On Thu, Jun 04, 2026 at 11:48:56AM -0400, Yazen Ghannam wrote:
> I think the issue is having concurrent updates to mci->error_desc. EDAC
> assumes a single owner for the MCIs. The fake_inject interface basically
> acts as a second module/owner.

Ok, something like this. Ugly but it should show the intent. It probably needs
more auditing whether we have copied all the fields we need down the
edac_mc_handle_error() path...

diff --git a/drivers/edac/debugfs.c b/drivers/edac/debugfs.c
index 8195fc9c9354..034b9b3f9cf5 100644
--- a/drivers/edac/debugfs.c
+++ b/drivers/edac/debugfs.c
@@ -14,28 +14,38 @@ static ssize_t edac_fake_inject_write(struct file *file,
 	struct mem_ctl_info *mci = to_mci(dev);
 	static enum hw_event_mc_err_type type;
 	u16 errcount = mci->fake_inject_count;
+	struct mem_ctl_info *imci;
+
+	imci = kmalloc(sizeof(struct mem_ctl_info), GFP_KERNEL);
+	if (!imci)
+		return -ENOMEM;
+
+	/* use a local copy and copy the struct */
+	*imci = *mci;
 
 	if (!errcount)
 		errcount = 1;
 
-	type = mci->fake_inject_ue ? HW_EVENT_ERR_UNCORRECTED
-				   : HW_EVENT_ERR_CORRECTED;
+	type = imci->fake_inject_ue ? HW_EVENT_ERR_UNCORRECTED
+				    : HW_EVENT_ERR_CORRECTED;
 
 	printk(KERN_DEBUG
 	       "Generating %d %s fake error%s to %d.%d.%d to test core handling. NOTE: this won't test the driver-specific decoding logic.\n",
 		errcount,
 		(type == HW_EVENT_ERR_UNCORRECTED) ? "UE" : "CE",
 		str_plural(errcount),
-		mci->fake_inject_layer[0],
-		mci->fake_inject_layer[1],
-		mci->fake_inject_layer[2]
+		imci->fake_inject_layer[0],
+		imci->fake_inject_layer[1],
+		imci->fake_inject_layer[2]
 	       );
-	edac_mc_handle_error(type, mci, errcount, 0, 0, 0,
-			     mci->fake_inject_layer[0],
-			     mci->fake_inject_layer[1],
-			     mci->fake_inject_layer[2],
+	edac_mc_handle_error(type, imci, errcount, 0, 0, 0,
+			     imci->fake_inject_layer[0],
+			     imci->fake_inject_layer[1],
+			     imci->fake_inject_layer[2],
 			     "FAKE ERROR", "for EDAC testing only");
 
+	kfree(imci);
+
 	return count;
 }
 


-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v5 0/2] Incorporate DRAM address in EDAC messages

[Yazen Ghannam]

On Tue, Jun 09, 2026 at 05:35:16PM -0700, Borislav Petkov wrote:
> On Thu, Jun 04, 2026 at 11:48:56AM -0400, Yazen Ghannam wrote:
> > I think the issue is having concurrent updates to mci->error_desc. EDAC
> > assumes a single owner for the MCIs. The fake_inject interface basically
> > acts as a second module/owner.
> 
> Ok, something like this. Ugly but it should show the intent. It probably needs
> more auditing whether we have copied all the fields we need down the
> edac_mc_handle_error() path...
> 
> diff --git a/drivers/edac/debugfs.c b/drivers/edac/debugfs.c
> index 8195fc9c9354..034b9b3f9cf5 100644
> --- a/drivers/edac/debugfs.c
> +++ b/drivers/edac/debugfs.c
> @@ -14,28 +14,38 @@ static ssize_t edac_fake_inject_write(struct file *file,
>  	struct mem_ctl_info *mci = to_mci(dev);
>  	static enum hw_event_mc_err_type type;
>  	u16 errcount = mci->fake_inject_count;
> +	struct mem_ctl_info *imci;
> +
> +	imci = kmalloc(sizeof(struct mem_ctl_info), GFP_KERNEL);
> +	if (!imci)
> +		return -ENOMEM;
> +
> +	/* use a local copy and copy the struct */
> +	*imci = *mci;
>  
>  	if (!errcount)
>  		errcount = 1;
>  
> -	type = mci->fake_inject_ue ? HW_EVENT_ERR_UNCORRECTED
> -				   : HW_EVENT_ERR_CORRECTED;
> +	type = imci->fake_inject_ue ? HW_EVENT_ERR_UNCORRECTED
> +				    : HW_EVENT_ERR_CORRECTED;
>  
>  	printk(KERN_DEBUG
>  	       "Generating %d %s fake error%s to %d.%d.%d to test core handling. NOTE: this won't test the driver-specific decoding logic.\n",
>  		errcount,
>  		(type == HW_EVENT_ERR_UNCORRECTED) ? "UE" : "CE",
>  		str_plural(errcount),
> -		mci->fake_inject_layer[0],
> -		mci->fake_inject_layer[1],
> -		mci->fake_inject_layer[2]
> +		imci->fake_inject_layer[0],
> +		imci->fake_inject_layer[1],
> +		imci->fake_inject_layer[2]
>  	       );
> -	edac_mc_handle_error(type, mci, errcount, 0, 0, 0,
> -			     mci->fake_inject_layer[0],
> -			     mci->fake_inject_layer[1],
> -			     mci->fake_inject_layer[2],
> +	edac_mc_handle_error(type, imci, errcount, 0, 0, 0,
> +			     imci->fake_inject_layer[0],
> +			     imci->fake_inject_layer[1],
> +			     imci->fake_inject_layer[2],
>  			     "FAKE ERROR", "for EDAC testing only");
>  
> +	kfree(imci);
> +
>  	return count;
>  }
>  
> 
> 

Right, this fixes the race, and printk/trace would be okay. But the
normal EDAC interface won't get updated. I think that breaks the fake
inject use case.

Ex.
1) Use fake inject to target layer/severity.
2) Check EDAC interface to see layer/severity counters increase.

I don't use the fake inject interface, so I may be mistaken about its
use.

Thanks,
Yazen

Re: [PATCH v5 0/2] Incorporate DRAM address in EDAC messages

[Borislav Petkov]

+ Mauro.

On Wed, Jun 10, 2026 at 05:15:41PM -0400, Yazen Ghannam wrote:
> On Tue, Jun 09, 2026 at 05:35:16PM -0700, Borislav Petkov wrote:
> > On Thu, Jun 04, 2026 at 11:48:56AM -0400, Yazen Ghannam wrote:
> > > I think the issue is having concurrent updates to mci->error_desc. EDAC
> > > assumes a single owner for the MCIs. The fake_inject interface basically
> > > acts as a second module/owner.
> > 
> > Ok, something like this. Ugly but it should show the intent. It probably needs
> > more auditing whether we have copied all the fields we need down the
> > edac_mc_handle_error() path...
> > 
> > diff --git a/drivers/edac/debugfs.c b/drivers/edac/debugfs.c
> > index 8195fc9c9354..034b9b3f9cf5 100644
> > --- a/drivers/edac/debugfs.c
> > +++ b/drivers/edac/debugfs.c
> > @@ -14,28 +14,38 @@ static ssize_t edac_fake_inject_write(struct file *file,
> >  	struct mem_ctl_info *mci = to_mci(dev);
> >  	static enum hw_event_mc_err_type type;
> >  	u16 errcount = mci->fake_inject_count;
> > +	struct mem_ctl_info *imci;
> > +
> > +	imci = kmalloc(sizeof(struct mem_ctl_info), GFP_KERNEL);
> > +	if (!imci)
> > +		return -ENOMEM;
> > +
> > +	/* use a local copy and copy the struct */
> > +	*imci = *mci;
> >  
> >  	if (!errcount)
> >  		errcount = 1;
> >  
> > -	type = mci->fake_inject_ue ? HW_EVENT_ERR_UNCORRECTED
> > -				   : HW_EVENT_ERR_CORRECTED;
> > +	type = imci->fake_inject_ue ? HW_EVENT_ERR_UNCORRECTED
> > +				    : HW_EVENT_ERR_CORRECTED;
> >  
> >  	printk(KERN_DEBUG
> >  	       "Generating %d %s fake error%s to %d.%d.%d to test core handling. NOTE: this won't test the driver-specific decoding logic.\n",
> >  		errcount,
> >  		(type == HW_EVENT_ERR_UNCORRECTED) ? "UE" : "CE",
> >  		str_plural(errcount),
> > -		mci->fake_inject_layer[0],
> > -		mci->fake_inject_layer[1],
> > -		mci->fake_inject_layer[2]
> > +		imci->fake_inject_layer[0],
> > +		imci->fake_inject_layer[1],
> > +		imci->fake_inject_layer[2]
> >  	       );
> > -	edac_mc_handle_error(type, mci, errcount, 0, 0, 0,
> > -			     mci->fake_inject_layer[0],
> > -			     mci->fake_inject_layer[1],
> > -			     mci->fake_inject_layer[2],
> > +	edac_mc_handle_error(type, imci, errcount, 0, 0, 0,
> > +			     imci->fake_inject_layer[0],
> > +			     imci->fake_inject_layer[1],
> > +			     imci->fake_inject_layer[2],
> >  			     "FAKE ERROR", "for EDAC testing only");
> >  
> > +	kfree(imci);
> > +
> >  	return count;
> >  }
> >  
> > 
> > 
> 
> Right, this fixes the race, and printk/trace would be okay. But the
> normal EDAC interface won't get updated. I think that breaks the fake
> inject use case.

You have the fake_inject fields, see edac_create_debugfs_nodes(). Those are
the fake_inject counters which should get incremented and I think we can make
it work by making the imci thing global and always hand it in to those
functions...

> Ex.
> 1) Use fake inject to target layer/severity.
> 2) Check EDAC interface to see layer/severity counters increase.
> 
> I don't use the fake inject interface, so I may be mistaken about its
> use.

... and neither do I. And I haven't heard of anyone doing so. And that's
perhaps the case because the thing is behind CONFIG_EDAC_DEBUG and only we
enable that.

So I'm thinking fake_inject has reached its retiring age. And 

452a6bf955ee ("edac: Add debufs nodes to allow doing fake error inject")

should simply be reverted.

Mauro's on Cc so that he can get a chance to chime in but lemme paste for him
the issue that Sashiko caught and that, AFAICT, has always been there:

---
> @@ -2748,7 +2768,7 @@ static void __log_ecc_error(struct mem_ctl_info *mci, struct err_info *err,
>  	edac_mc_handle_error(err_type, mci, 1,
>  			     err->page, err->offset, err->syndrome,
>  			     err->csrow, err->channel, -1,
> -			     string, "");
> +			     string, s);
Could passing a stack-allocated buffer to edac_mc_handle_error() risk a
use-after-free of stack memory?
Looking at edac_mc_handle_error(), it directly assigns the other_detail
pointer to the globally shared error_desc structure without holding locks:
drivers/edac/edac_mc.c:edac_mc_handle_error() {
    ...
    e->other_detail = other_detail ?: "";
    ...
}
While normal hardware MCE decoding is serialized by the mce_work workqueue,
the EDAC fake_inject debugfs interface can execute concurrently.
Could a user writing to fake_inject race to modify mci->error_desc?
Thread 1 (mce_work):
__log_ecc_error()
    char s[MSG_SIZE];
    edac_mc_handle_error(..., s)
        e->other_detail = s;
If __log_ecc_error() returns and its stack frame is popped, and Thread 2
(fake_inject) is concurrently executing edac_raw_mc_handle_error(), could
it read the dangling pointer from e->other_detail?
---

(from https://sashiko.dev/#/patchset/20260518160716.171578-1-yazen.ghannam%40amd.com)

If this were to be done right, then that passing in of a string buffer
allocated in the mci would mean that all those functions working on it down
the callpath cannot ever be called concurrently.

Or if they have to, a separate mci would have to be handed in. Either way,
that would need hairy rework and right now, the simplest thing to do is simply
zap that fake_inject stuff which is not used anyway.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v5 0/2] Incorporate DRAM address in EDAC messages

[Yazen Ghannam]

On Wed, Jun 10, 2026 at 06:23:36PM -0700, Borislav Petkov wrote:
> + Mauro.
> 
> On Wed, Jun 10, 2026 at 05:15:41PM -0400, Yazen Ghannam wrote:
> > On Tue, Jun 09, 2026 at 05:35:16PM -0700, Borislav Petkov wrote:
> > > On Thu, Jun 04, 2026 at 11:48:56AM -0400, Yazen Ghannam wrote:
> > > > I think the issue is having concurrent updates to mci->error_desc. EDAC
> > > > assumes a single owner for the MCIs. The fake_inject interface basically
> > > > acts as a second module/owner.
> > > 
> > > Ok, something like this. Ugly but it should show the intent. It probably needs
> > > more auditing whether we have copied all the fields we need down the
> > > edac_mc_handle_error() path...
> > > 
> > > diff --git a/drivers/edac/debugfs.c b/drivers/edac/debugfs.c
> > > index 8195fc9c9354..034b9b3f9cf5 100644
> > > --- a/drivers/edac/debugfs.c
> > > +++ b/drivers/edac/debugfs.c
> > > @@ -14,28 +14,38 @@ static ssize_t edac_fake_inject_write(struct file *file,
> > >  	struct mem_ctl_info *mci = to_mci(dev);
> > >  	static enum hw_event_mc_err_type type;
> > >  	u16 errcount = mci->fake_inject_count;
> > > +	struct mem_ctl_info *imci;
> > > +
> > > +	imci = kmalloc(sizeof(struct mem_ctl_info), GFP_KERNEL);
> > > +	if (!imci)
> > > +		return -ENOMEM;
> > > +
> > > +	/* use a local copy and copy the struct */
> > > +	*imci = *mci;
> > >  
> > >  	if (!errcount)
> > >  		errcount = 1;
> > >  
> > > -	type = mci->fake_inject_ue ? HW_EVENT_ERR_UNCORRECTED
> > > -				   : HW_EVENT_ERR_CORRECTED;
> > > +	type = imci->fake_inject_ue ? HW_EVENT_ERR_UNCORRECTED
> > > +				    : HW_EVENT_ERR_CORRECTED;
> > >  
> > >  	printk(KERN_DEBUG
> > >  	       "Generating %d %s fake error%s to %d.%d.%d to test core handling. NOTE: this won't test the driver-specific decoding logic.\n",
> > >  		errcount,
> > >  		(type == HW_EVENT_ERR_UNCORRECTED) ? "UE" : "CE",
> > >  		str_plural(errcount),
> > > -		mci->fake_inject_layer[0],
> > > -		mci->fake_inject_layer[1],
> > > -		mci->fake_inject_layer[2]
> > > +		imci->fake_inject_layer[0],
> > > +		imci->fake_inject_layer[1],
> > > +		imci->fake_inject_layer[2]
> > >  	       );
> > > -	edac_mc_handle_error(type, mci, errcount, 0, 0, 0,
> > > -			     mci->fake_inject_layer[0],
> > > -			     mci->fake_inject_layer[1],
> > > -			     mci->fake_inject_layer[2],
> > > +	edac_mc_handle_error(type, imci, errcount, 0, 0, 0,
> > > +			     imci->fake_inject_layer[0],
> > > +			     imci->fake_inject_layer[1],
> > > +			     imci->fake_inject_layer[2],
> > >  			     "FAKE ERROR", "for EDAC testing only");
> > >  
> > > +	kfree(imci);
> > > +
> > >  	return count;
> > >  }
> > >  
> > > 
> > > 
> > 
> > Right, this fixes the race, and printk/trace would be okay. But the
> > normal EDAC interface won't get updated. I think that breaks the fake
> > inject use case.
> 
> You have the fake_inject fields, see edac_create_debugfs_nodes(). Those are
> the fake_inject counters which should get incremented and I think we can make
> it work by making the imci thing global and always hand it in to those
> functions...
> 

The fake_inject fields are targets.

Ex. under '/sys/debug/kernel/edac/mc0'
fake_inject_count=3
fake_inject_channel=1
fake_inject_csrow=2

...means inject 3 correctable errors to mc0/channel1/csrow2.

This would be reflected in
'/sys/devices/system/edac/mc/mc0/rankX/dimm_ce_count'

rankX = whatever rank the channel/csrow map to.

OFC, the layers are generic. They're based on whatever the EDAC module
uses.

> > Ex.
> > 1) Use fake inject to target layer/severity.
> > 2) Check EDAC interface to see layer/severity counters increase.
> > 
> > I don't use the fake inject interface, so I may be mistaken about its
> > use.
> 
> ... and neither do I. And I haven't heard of anyone doing so. And that's
> perhaps the case because the thing is behind CONFIG_EDAC_DEBUG and only we
> enable that.
> 
> So I'm thinking fake_inject has reached its retiring age. And 
> 
> 452a6bf955ee ("edac: Add debufs nodes to allow doing fake error inject")
> 
> should simply be reverted.
> 
> Mauro's on Cc so that he can get a chance to chime in but lemme paste for him
> the issue that Sashiko caught and that, AFAICT, has always been there:

Right, this isn't new behavior. That's why I don't think it's a real
issue. It's a corner case for a specific testing environment.

> 
> ---
> > @@ -2748,7 +2768,7 @@ static void __log_ecc_error(struct mem_ctl_info *mci, struct err_info *err,
> >  	edac_mc_handle_error(err_type, mci, 1,
> >  			     err->page, err->offset, err->syndrome,
> >  			     err->csrow, err->channel, -1,
> > -			     string, "");
> > +			     string, s);
> Could passing a stack-allocated buffer to edac_mc_handle_error() risk a
> use-after-free of stack memory?
> Looking at edac_mc_handle_error(), it directly assigns the other_detail
> pointer to the globally shared error_desc structure without holding locks:
> drivers/edac/edac_mc.c:edac_mc_handle_error() {
>     ...
>     e->other_detail = other_detail ?: "";
>     ...
> }
> While normal hardware MCE decoding is serialized by the mce_work workqueue,
> the EDAC fake_inject debugfs interface can execute concurrently.
> Could a user writing to fake_inject race to modify mci->error_desc?
> Thread 1 (mce_work):
> __log_ecc_error()
>     char s[MSG_SIZE];
>     edac_mc_handle_error(..., s)
>         e->other_detail = s;
> If __log_ecc_error() returns and its stack frame is popped, and Thread 2
> (fake_inject) is concurrently executing edac_raw_mc_handle_error(), could
> it read the dangling pointer from e->other_detail?
> ---
> 
> (from https://sashiko.dev/#/patchset/20260518160716.171578-1-yazen.ghannam%40amd.com)
> 
> If this were to be done right, then that passing in of a string buffer
> allocated in the mci would mean that all those functions working on it down
> the callpath cannot ever be called concurrently.
> 
> Or if they have to, a separate mci would have to be handed in. Either way,
> that would need hairy rework and right now, the simplest thing to do is simply
> zap that fake_inject stuff which is not used anyway.
> 

Just curious, why not leave it? Do you think the bots will keep bringing
it up?

Thanks,
Yazen

Re: [PATCH v5 0/2] Incorporate DRAM address in EDAC messages

[Borislav Petkov]

On Thu, Jun 11, 2026 at 10:11:38AM -0400, Yazen Ghannam wrote:
> The fake_inject fields are targets.

Uff, true story, I got confused, sorry.

/facepalm.

> Just curious, why not leave it? Do you think the bots will keep bringing
> it up?

Ok, someone tells you about a bug in the software. You do nothing?

When do you fix it if you intend to do so?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v5 0/2] Incorporate DRAM address in EDAC messages

[Yazen Ghannam]

On Thu, Jun 11, 2026 at 09:16:11AM -0700, Borislav Petkov wrote:
> On Thu, Jun 11, 2026 at 10:11:38AM -0400, Yazen Ghannam wrote:
> > The fake_inject fields are targets.
> 
> Uff, true story, I got confused, sorry.
> 
> /facepalm.
> 
> > Just curious, why not leave it? Do you think the bots will keep bringing
> > it up?
> 
> Ok, someone tells you about a bug in the software. You do nothing?
> 
> When do you fix it if you intend to do so?
> 

IMO, consider the risk and the outcome.

In this case, the conditions to hit the issue are rare.

Real DRAM ECC occurs and logged through EDAC: rare
EDAC_DEBUG config option enabled: almost never
User is doing a fake_inject: practically 0% chance

If the issue occurs, what is the outcome?

The "other_detail" string gets messed up for one of the error logs.

Of course, if no one is using fake_inject, then removing it closes this
small risk. I guess that's the point, right?

Thanks,
Yazen

Re: [PATCH v5 0/2] Incorporate DRAM address in EDAC messages

[Borislav Petkov]

On Thu, Jun 11, 2026 at 01:01:07PM -0400, Yazen Ghannam wrote:
> IMO, consider the risk and the outcome.
>
> In this case, the conditions to hit the issue are rare.
> 
> Real DRAM ECC occurs and logged through EDAC: rare
> EDAC_DEBUG config option enabled: almost never
> User is doing a fake_inject: practically 0% chance
> 
> If the issue occurs, what is the outcome?
> 
> The "other_detail" string gets messed up for one of the error logs.
> 
> Of course, if no one is using fake_inject, then removing it closes this
> small risk. I guess that's the point, right?

We don't ever keep broken code in the kernel. No matter how many people can
hit it. I know, I know, but what is the chance?

It doesn't matter. If we know it is broken we either fix it or remove it. And
sometimes fixing it "takes a while" and there are bugs which linger for years
for whatever reason.

But we never leave stuff unfixed because someone might use it, someone might
depend on it and considering how many people are using Linux, you can imagine
that "practically 0% chance" is more than 0 in reality...

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette