[PATCH v3] EDAC/versalnet: Report PFN and page offset for DDR errors

[PATCH v3] EDAC/versalnet: Report PFN and page offset for DDR errors

[Shubhrajyoti Datta]

Populate the EDAC location fields when reporting DDRMC CE/UE events.

Compute the physical address from the decoded error info and pass the
PFN along with the offset-within-page to edac_mc_handle_error(). This
allows user space (mc_event consumers) to correlate memory errors with
a real address.

Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
Reviewed-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
---

Changes in v3:
Update the comment for DDR address

Changes in v2:
Refactor the common code

 drivers/edac/versalnet_edac.c | 36 +++++++++++++++++------------------
 1 file changed, 17 insertions(+), 19 deletions(-)

diff --git a/drivers/edac/versalnet_edac.c b/drivers/edac/versalnet_edac.c
index ec1315582414..7a10c53c4768 100644
--- a/drivers/edac/versalnet_edac.c
+++ b/drivers/edac/versalnet_edac.c
@@ -265,7 +265,7 @@ static bool get_ddr_info(u32 *error_data, struct mc_priv *priv)
  * @controller:	Controller number of the MC5
  * @error_data:	the DDRMC5 ADEC address decoder register data
  *
- * Return: physical address of the DDR memory.
+ * Returns the system physical address corresponding to the reported DDR
  */
 static unsigned long convert_to_physical(struct mc_priv *priv,
 					 union ecc_error_info pinf,
@@ -429,6 +429,7 @@ static unsigned long convert_to_physical(struct mc_priv *priv,
 static void handle_error(struct mc_priv  *priv, struct ecc_status *stat,
 			 int ctl_num, int *error_data)
 {
+	enum hw_event_mc_err_type type;
 	union ecc_error_info pinf;
 	struct mem_ctl_info *mci;
 	unsigned long pa;
@@ -440,29 +441,26 @@ static void handle_error(struct mc_priv  *priv, struct ecc_status *stat,
 
 	mci = priv->mci[ctl_num];
 
-	if (stat->error_type == MC5_ERR_TYPE_CE) {
+	if (stat->error_type == MC5_ERR_TYPE_UE) {
+		pinf = stat->ueinfo[stat->channel];
+		type = HW_EVENT_ERR_UNCORRECTED;
+	} else {
 		pinf = stat->ceinfo[stat->channel];
-		snprintf(priv->message, sizeof(priv->message),
-			 "Error type:%s Controller %d Addr at %lx\n",
-			 "CE", ctl_num, convert_to_physical(priv, pinf, ctl_num, error_data));
-
-		edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci,
-				     1, 0, 0, 0, 0, 0, -1,
-				     priv->message, "");
+		type = HW_EVENT_ERR_CORRECTED;
 	}
 
-	if (stat->error_type == MC5_ERR_TYPE_UE) {
-		pinf = stat->ueinfo[stat->channel];
-		snprintf(priv->message, sizeof(priv->message),
-			 "Error type:%s controller %d Addr at %lx\n",
-			 "UE", ctl_num, convert_to_physical(priv, pinf, ctl_num, error_data));
+	pa = convert_to_physical(priv, pinf, ctl_num, error_data);
+	pfn = PHYS_PFN(pa);
+	snprintf(priv->message, sizeof(priv->message),
+		 "Error type:%s Controller %d Addr at %lx\n",
+		 type == HW_EVENT_ERR_UNCORRECTED ? "UE" : "CE",
+		 ctl_num, pa);
 
-		edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci,
-				     1, 0, 0, 0, 0, 0, -1,
-				     priv->message, "");
-		pa = convert_to_physical(priv, pinf, ctl_num, error_data);
-		pfn = PHYS_PFN(pa);
+	edac_mc_handle_error(type, mci,
+			     1, pfn, offset_in_page(pa), 0, 0, 0, -1,
+			     priv->message, "");
 
+	if (stat->error_type == MC5_ERR_TYPE_UE) {
 		if (IS_ENABLED(CONFIG_MEMORY_FAILURE)) {
 			err = memory_failure(pfn, MF_ACTION_REQUIRED);
 			if (err)
-- 
2.49.1

Re: [PATCH v3] EDAC/versalnet: Report PFN and page offset for DDR errors

[Borislav Petkov]

On Mon, May 11, 2026 at 04:05:09PM +0530, Shubhrajyoti Datta wrote:
> Populate the EDAC location fields when reporting DDRMC CE/UE events.
> 
> Compute the physical address from the decoded error info and pass the
> PFN along with the offset-within-page to edac_mc_handle_error(). This
> allows user space (mc_event consumers) to correlate memory errors with
> a real address.
> 
> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
> Reviewed-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> ---
> 
> Changes in v3:
> Update the comment for DDR address
> 
> Changes in v2:
> Refactor the common code
> 
>  drivers/edac/versalnet_edac.c | 36 +++++++++++++++++------------------
>  1 file changed, 17 insertions(+), 19 deletions(-)

https://sashiko.dev/#/patchset/20260511103529.1853270-1-shubhrajyoti.datta%40amd.com

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v3] EDAC/versalnet: Report PFN and page offset for DDR errors

[Datta, Shubhrajyoti]

Public

 > -----Original Message-----
 > From: Borislav Petkov <bp@alien8.de>
 > Sent: Thursday, May 14, 2026 5:05 PM
 > To: Datta, Shubhrajyoti <shubhrajyoti.datta@amd.com>
 > Cc: linux-kernel@vger.kernel.org; linux-edac@vger.kernel.org; git
(AMD-Xilinx)
 > <git@amd.com>; shubhrajyoti.datta@gmail.com; Tony Luck
 > <tony.luck@intel.com>; Potthuri, Sai Krishna
<sai.krishna.potthuri@amd.com>
 > Subject: Re: [PATCH v3] EDAC/versalnet: Report PFN and page offset
for DDR
...
 >
 > https://sashiko.dev/#/patchset/20260511103529.1853270-1-
 > shubhrajyoti.datta%40amd.com
 >
Hi Boris,
I have reviewed all the Sashiko comments and identified the valid ones
(details below).

Finding 1: Missing null check for

Comment is accepted will send a patch.
Finding 2: Uninitialized ueinfo when CE and UE flags set simultaneously -- Mostly a false positive

Looking at rpmsg_cb, CE (error_id 18) and UE (error_id 19) arrive as separate rpmsg messages with distinct error_id values.
The memset(p, 0, ...) at the top of each case clears the entire ecc_status
  struct, and p->error_type is set to exactly one of MC5_ERR_TYPE_CE or MC5_ERR_TYPE_UE.
The get_ddr_info() function then uses if/else if on ISR bits, but the ISR comes from the firmware message for
  that specific error_id, so only one type should be populated per invocation.

  Finding 3: memory_failure() with MF_ACTION_REQUIRED from async context


Sent below patch.
https://lore.kernel.org/all/20260701095040.4139675-1-shubhrajyoti.datta@amd.com/

Regarding the context: rpmsg_cb is a virtio rpmsg callback. Looking at the virtio rpmsg transport, these callbacks run from
a workqueue (virtio_rpmsg_bus.c uses rpmsg_recv_single() from
  rpmsg_recv_done() which is a virtqueue callback typically dispatched via a workqueue),
so it's not hardirq context. memory_failure() does take mf_mutex which can sleep, and this should be fine from a
  workqueue.

Also the cdx code holds a mutex
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/cdx/controller/cdx_rpmsg.c?h=v7.2-rc1#n106

cdx_rpmsg_cb -> cdx_mcdi_process_cmd -> holds mutex

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/cdx/controller/mcdi.c?h=v7.2-rc1#n611

This looks like a  a false positive.


 > --
 > Regards/Gruss,
 >     Boris.
 >
 > https://people.kernel.org/tglx/notes-about-netiquette