[PATCH v2] EDAC/versalnet: Refactor memory controller initialization and cleanup

[PATCH v2] EDAC/versalnet: Refactor memory controller initialization and cleanup

[Shubhrajyoti Datta]

Simplify the initialization and cleanup flow for Versal Net DDRMC
controllers in the EDAC driver.

Introduce `init_single_versalnet()` for per-controller setup and
  `init_versalnet()` for looping through NUM_CONTROLLERS, also add
rollback logic to handle  partial init failures.

Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
---

Changes in v2:
- Rename init_single_versalnet() to init_mc() for clarity.
- Rename remove_single_versalnet() to remove_mc() to match naming convention.
- Simplify error handling in init_versalnet() by replacing goto with a rollback loop.
- Reduce indentation and consolidate cleanup logic.

 drivers/edac/versalnet_edac.c | 156 ++++++++++++++++++----------------
 1 file changed, 83 insertions(+), 73 deletions(-)

diff --git a/drivers/edac/versalnet_edac.c b/drivers/edac/versalnet_edac.c
index 1ded4c3f0213..01edc7408a5c 100644
--- a/drivers/edac/versalnet_edac.c
+++ b/drivers/edac/versalnet_edac.c
@@ -758,7 +758,17 @@ static void versal_edac_release(struct device *dev)
 	kfree(dev);
 }
 
-static int init_versalnet(struct mc_priv *priv, struct platform_device *pdev)
+static void remove_mc(struct mc_priv *priv, int i)
+{
+	struct mem_ctl_info *mci;
+
+	mci = priv->mci[i];
+	device_unregister(mci->pdev);
+	edac_mc_del_mc(mci->pdev);
+	edac_mc_free(mci);
+}
+
+static int init_mc(struct mc_priv *priv, struct platform_device *pdev, int i)
 {
 	u32 num_chans, rank, dwidth, config;
 	struct edac_mc_layer layers[2];
@@ -766,87 +776,90 @@ static int init_versalnet(struct mc_priv *priv, struct platform_device *pdev)
 	struct device *dev;
 	enum dev_type dt;
 	char *name;
-	int rc, i;
-
-	for (i = 0; i < NUM_CONTROLLERS; i++) {
-		config = priv->adec[CONF + i * ADEC_NUM];
-		num_chans = FIELD_GET(MC5_NUM_CHANS_MASK, config);
-		rank = 1 << FIELD_GET(MC5_RANK_MASK, config);
-		dwidth = FIELD_GET(MC5_BUS_WIDTH_MASK, config);
-
-		switch (dwidth) {
-		case XDDR5_BUS_WIDTH_16:
-			dt = DEV_X16;
-			break;
-		case XDDR5_BUS_WIDTH_32:
-			dt = DEV_X32;
-			break;
-		case XDDR5_BUS_WIDTH_64:
-			dt = DEV_X64;
-			break;
-		default:
-			dt = DEV_UNKNOWN;
-		}
+	int rc;
 
-		if (dt == DEV_UNKNOWN)
-			continue;
+	config = priv->adec[CONF + i * ADEC_NUM];
+	num_chans = FIELD_GET(MC5_NUM_CHANS_MASK, config);
+	rank = 1 << FIELD_GET(MC5_RANK_MASK, config);
+	dwidth = FIELD_GET(MC5_BUS_WIDTH_MASK, config);
+
+	switch (dwidth) {
+	case XDDR5_BUS_WIDTH_16:
+		dt = DEV_X16;
+		break;
+	case XDDR5_BUS_WIDTH_32:
+		dt = DEV_X32;
+		break;
+	case XDDR5_BUS_WIDTH_64:
+		dt = DEV_X64;
+		break;
+	default:
+		dt = DEV_UNKNOWN;
+	}
 
-		/* Find the first enabled device and register that one. */
-		layers[0].type = EDAC_MC_LAYER_CHIP_SELECT;
-		layers[0].size = rank;
-		layers[0].is_virt_csrow = true;
-		layers[1].type = EDAC_MC_LAYER_CHANNEL;
-		layers[1].size = num_chans;
-		layers[1].is_virt_csrow = false;
+	if (dt == DEV_UNKNOWN)
+		return 0;
 
-		rc = -ENOMEM;
-		mci = edac_mc_alloc(i, ARRAY_SIZE(layers), layers,
-				    sizeof(struct mc_priv));
-		if (!mci) {
-			edac_printk(KERN_ERR, EDAC_MC, "Failed memory allocation for MC%d\n", i);
-			goto err_alloc;
-		}
+	/* Find the first enabled device and register that one. */
+	layers[0].type = EDAC_MC_LAYER_CHIP_SELECT;
+	layers[0].size = rank;
+	layers[0].is_virt_csrow = true;
+	layers[1].type = EDAC_MC_LAYER_CHANNEL;
+	layers[1].size = num_chans;
+	layers[1].is_virt_csrow = false;
+
+	rc = -ENOMEM;
+	mci = edac_mc_alloc(i, ARRAY_SIZE(layers), layers,
+			    sizeof(struct mc_priv));
+	if (!mci) {
+		edac_printk(KERN_ERR, EDAC_MC, "Failed memory allocation for MC%d\n", i);
+		return rc;
+	}
+	priv->mci[i] = mci;
+	priv->dwidth = dt;
+
+	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+	if (!dev)
+		goto err_mc_free;
+	dev->release = versal_edac_release;
+	name = kmalloc(32, GFP_KERNEL);
+	sprintf(name, "versal-net-ddrmc5-edac-%d", i);
+	dev->init_name = name;
+	rc = device_register(dev);
+	if (rc)
+		goto err_mc_free;
 
-		priv->mci[i] = mci;
-		priv->dwidth = dt;
+	mci->pdev = dev;
 
-		dev = kzalloc(sizeof(*dev), GFP_KERNEL);
-		dev->release = versal_edac_release;
-		name = kmalloc(32, GFP_KERNEL);
-		sprintf(name, "versal-net-ddrmc5-edac-%d", i);
-		dev->init_name = name;
-		rc = device_register(dev);
-		if (rc)
-			goto err_alloc;
+	platform_set_drvdata(pdev, priv);
 
-		mci->pdev = dev;
+	mc_init(mci, dev);
+	rc = edac_mc_add_mc(mci);
+	if (rc) {
+		edac_printk(KERN_ERR, EDAC_MC, "Failed to register MC%d with EDAC core\n", i);
+		goto err_unreg;
+	}
+	return 0;
+err_unreg:
+	device_unregister(mci->pdev);
+err_mc_free:
+	edac_mc_free(mci);
+	return rc;
+}
 
-		platform_set_drvdata(pdev, priv);
+static int init_versalnet(struct mc_priv *priv, struct platform_device *pdev)
+{
+	int rc, i;
 
-		mc_init(mci, dev);
-		rc = edac_mc_add_mc(mci);
+	for (i = 0; i < NUM_CONTROLLERS; i++) {
+		rc = init_mc(priv, pdev, i);
 		if (rc) {
-			edac_printk(KERN_ERR, EDAC_MC, "Failed to register MC%d with EDAC core\n", i);
-			goto err_alloc;
+			while (i--)
+				remove_mc(priv, i);
+			return rc;
 		}
 	}
 	return 0;
-
-err_alloc:
-	while (i--) {
-		mci = priv->mci[i];
-		if (!mci)
-			continue;
-
-		if (mci->pdev) {
-			device_unregister(mci->pdev);
-			edac_mc_del_mc(mci->pdev);
-		}
-
-		edac_mc_free(mci);
-	}
-
-	return rc;
 }
 
 static void remove_versalnet(struct mc_priv *priv)
@@ -857,9 +870,6 @@ static void remove_versalnet(struct mc_priv *priv)
 	for (i = 0; i < NUM_CONTROLLERS; i++) {
 		device_unregister(priv->mci[i]->pdev);
 		mci = edac_mc_del_mc(priv->mci[i]->pdev);
-		if (!mci)
-			return;
-
 		edac_mc_free(mci);
 	}
 }
-- 
2.34.1

Re: [PATCH v2] EDAC/versalnet: Refactor memory controller initialization and cleanup

[Borislav Petkov]

On Tue, Nov 04, 2025 at 03:09:20PM +0530, Shubhrajyoti Datta wrote:
> Simplify the initialization and cleanup flow for Versal Net DDRMC
> controllers in the EDAC driver.
> 
> Introduce `init_single_versalnet()` for per-controller setup and
>   `init_versalnet()` for looping through NUM_CONTROLLERS, also add
> rollback logic to handle  partial init failures.
> 
> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
> ---
> 
> Changes in v2:
> - Rename init_single_versalnet() to init_mc() for clarity.
> - Rename remove_single_versalnet() to remove_mc() to match naming convention.
> - Simplify error handling in init_versalnet() by replacing goto with a rollback loop.
> - Reduce indentation and consolidate cleanup logic.

Better, here's some more improvements and cleanups ontop. You probably should
apply the diff to better see what I mean:

- do the kzalloc allocations first
- publish the structures only after they've been initialized properly so that
  you don't need to unwind unnecessarily when it fails later
- remove_versalnet() is now trivial

Do run it on the hw and have the code fail at certain places on purpose to
make sure the unwinding happens properly.

HTH.

---

diff --git a/drivers/edac/versalnet_edac.c b/drivers/edac/versalnet_edac.c
index 01edc7408a5c..dc6108f7cee3 100644
--- a/drivers/edac/versalnet_edac.c
+++ b/drivers/edac/versalnet_edac.c
@@ -70,6 +70,8 @@
 #define XDDR5_BUS_WIDTH_32		1
 #define XDDR5_BUS_WIDTH_16		2
 
+#define MC_NAME_LEN			32
+
 /**
  * struct ecc_error_info - ECC error log information.
  * @burstpos:		Burst position.
@@ -758,7 +760,7 @@ static void versal_edac_release(struct device *dev)
 	kfree(dev);
 }
 
-static void remove_mc(struct mc_priv *priv, int i)
+static void remove_one_mc(struct mc_priv *priv, int i)
 {
 	struct mem_ctl_info *mci;
 
@@ -768,7 +770,7 @@ static void remove_mc(struct mc_priv *priv, int i)
 	edac_mc_free(mci);
 }
 
-static int init_mc(struct mc_priv *priv, struct platform_device *pdev, int i)
+static int init_one_mc(struct mc_priv *priv, struct platform_device *pdev, int i)
 {
 	u32 num_chans, rank, dwidth, config;
 	struct edac_mc_layer layers[2];
@@ -809,41 +811,54 @@ static int init_mc(struct mc_priv *priv, struct platform_device *pdev, int i)
 	layers[1].is_virt_csrow = false;
 
 	rc = -ENOMEM;
-	mci = edac_mc_alloc(i, ARRAY_SIZE(layers), layers,
-			    sizeof(struct mc_priv));
-	if (!mci) {
-		edac_printk(KERN_ERR, EDAC_MC, "Failed memory allocation for MC%d\n", i);
+	name = kzalloc(MC_NAME_LEN, GFP_KERNEL);
+	if (!name)
 		return rc;
-	}
-	priv->mci[i] = mci;
-	priv->dwidth = dt;
 
 	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
 	if (!dev)
-		goto err_mc_free;
-	dev->release = versal_edac_release;
-	name = kmalloc(32, GFP_KERNEL);
+		goto err_name_free;
+
+	mci = edac_mc_alloc(i, ARRAY_SIZE(layers), layers, sizeof(struct mc_priv));
+	if (!mci) {
+		edac_printk(KERN_ERR, EDAC_MC, "Failed memory allocation for MC%d\n", i);
+		goto err_dev_free;
+	}
+
 	sprintf(name, "versal-net-ddrmc5-edac-%d", i);
+
 	dev->init_name = name;
+	dev->release = versal_edac_release;
+
 	rc = device_register(dev);
 	if (rc)
 		goto err_mc_free;
 
 	mci->pdev = dev;
-
-	platform_set_drvdata(pdev, priv);
-
 	mc_init(mci, dev);
+
 	rc = edac_mc_add_mc(mci);
 	if (rc) {
 		edac_printk(KERN_ERR, EDAC_MC, "Failed to register MC%d with EDAC core\n", i);
 		goto err_unreg;
 	}
+
+	priv->mci[i] = mci;
+	priv->dwidth = dt;
+
+	platform_set_drvdata(pdev, priv);
+
 	return 0;
+
 err_unreg:
 	device_unregister(mci->pdev);
 err_mc_free:
 	edac_mc_free(mci);
+err_dev_free:
+	kfree(dev);
+err_name_free:
+	kfree(name);
+
 	return rc;
 }
 
@@ -852,10 +867,10 @@ static int init_versalnet(struct mc_priv *priv, struct platform_device *pdev)
 	int rc, i;
 
 	for (i = 0; i < NUM_CONTROLLERS; i++) {
-		rc = init_mc(priv, pdev, i);
+		rc = init_one_mc(priv, pdev, i);
 		if (rc) {
 			while (i--)
-				remove_mc(priv, i);
+				remove_one_mc(priv, i);
 			return rc;
 		}
 	}
@@ -864,14 +879,8 @@ static int init_versalnet(struct mc_priv *priv, struct platform_device *pdev)
 
 static void remove_versalnet(struct mc_priv *priv)
 {
-	struct mem_ctl_info *mci;
-	int i;
-
-	for (i = 0; i < NUM_CONTROLLERS; i++) {
-		device_unregister(priv->mci[i]->pdev);
-		mci = edac_mc_del_mc(priv->mci[i]->pdev);
-		edac_mc_free(mci);
-	}
+	for (int i = 0; i < NUM_CONTROLLERS; i++)
+		remove_one_mc(priv, i);
 }
 
 static int mc_probe(struct platform_device *pdev)

-- 
Regards/Gruss,
    Boris.

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

RE: [PATCH v2] EDAC/versalnet: Refactor memory controller initialization and cleanup

[Datta, Shubhrajyoti]

[AMD Official Use Only - AMD Internal Distribution Only]

> -----Original Message-----
> From: Borislav Petkov <bp@alien8.de>
> Sent: Sunday, November 9, 2025 9:29 PM
> To: Datta, Shubhrajyoti <shubhrajyoti.datta@amd.com>
> Cc: linux-kernel@vger.kernel.org; linux-edac@vger.kernel.org;
> shubhrajyoti.datta@gmail.com; Tony Luck <tony.luck@intel.com>; James Morse
> <james.morse@arm.com>; Mauro Carvalho Chehab <mchehab@kernel.org>;
> Robert Richter <rric@kernel.org>
> Subject: Re: [PATCH v2] EDAC/versalnet: Refactor memory controller
> initialization and cleanup
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> On Tue, Nov 04, 2025 at 03:09:20PM +0530, Shubhrajyoti Datta wrote:
> > Simplify the initialization and cleanup flow for Versal Net DDRMC
> > controllers in the EDAC driver.
> >
> > Introduce `init_single_versalnet()` for per-controller setup and
> >   `init_versalnet()` for looping through NUM_CONTROLLERS, also add
> > rollback logic to handle  partial init failures.
> >
> > Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
> > ---
> >
> > Changes in v2:
> > - Rename init_single_versalnet() to init_mc() for clarity.
> > - Rename remove_single_versalnet() to remove_mc() to match naming
> convention.
> > - Simplify error handling in init_versalnet() by replacing goto with a rollback
> loop.
> > - Reduce indentation and consolidate cleanup logic.
>
> Better, here's some more improvements and cleanups ontop. You probably
> should apply the diff to better see what I mean:
>
> - do the kzalloc allocations first
> - publish the structures only after they've been initialized properly so that
>   you don't need to unwind unnecessarily when it fails later
> - remove_versalnet() is now trivial
>
> Do run it on the hw and have the code fail at certain places on purpose to make
> sure the unwinding happens properly.

Tested on the hardware.

>
> HTH.
>
> ---
>
> diff --git a/drivers/edac/versalnet_edac.c b/drivers/edac/versalnet_edac.c index
> 01edc7408a5c..dc6108f7cee3 100644
> --- a/drivers/edac/versalnet_edac.c
> +++ b/drivers/edac/versalnet_edac.c
> @@ -70,6 +70,8 @@
>  #define XDDR5_BUS_WIDTH_32             1
>  #define XDDR5_BUS_WIDTH_16             2
>
> +#define MC_NAME_LEN                    32
> +
>  /**
>   * struct ecc_error_info - ECC error log information.
>   * @burstpos:          Burst position.
> @@ -758,7 +760,7 @@ static void versal_edac_release(struct device *dev)
>         kfree(dev);
>  }
>
> -static void remove_mc(struct mc_priv *priv, int i)
> +static void remove_one_mc(struct mc_priv *priv, int i)
>  {
>         struct mem_ctl_info *mci;
>
> @@ -768,7 +770,7 @@ static void remove_mc(struct mc_priv *priv, int i)
>         edac_mc_free(mci);
>  }
>
> -static int init_mc(struct mc_priv *priv, struct platform_device *pdev, int i)
> +static int init_one_mc(struct mc_priv *priv, struct platform_device
> +*pdev, int i)
>  {
>         u32 num_chans, rank, dwidth, config;
>         struct edac_mc_layer layers[2];
> @@ -809,41 +811,54 @@ static int init_mc(struct mc_priv *priv, struct
> platform_device *pdev, int i)
>         layers[1].is_virt_csrow = false;
>
>         rc = -ENOMEM;
> -       mci = edac_mc_alloc(i, ARRAY_SIZE(layers), layers,
> -                           sizeof(struct mc_priv));
> -       if (!mci) {
> -               edac_printk(KERN_ERR, EDAC_MC, "Failed memory allocation for
> MC%d\n", i);
> +       name = kzalloc(MC_NAME_LEN, GFP_KERNEL);
> +       if (!name)
>                 return rc;
> -       }
> -       priv->mci[i] = mci;
> -       priv->dwidth = dt;
>
>         dev = kzalloc(sizeof(*dev), GFP_KERNEL);
>         if (!dev)
> -               goto err_mc_free;
> -       dev->release = versal_edac_release;
> -       name = kmalloc(32, GFP_KERNEL);
> +               goto err_name_free;
> +
> +       mci = edac_mc_alloc(i, ARRAY_SIZE(layers), layers, sizeof(struct mc_priv));
> +       if (!mci) {
> +               edac_printk(KERN_ERR, EDAC_MC, "Failed memory allocation for
> MC%d\n", i);
> +               goto err_dev_free;
> +       }
> +
>         sprintf(name, "versal-net-ddrmc5-edac-%d", i);
> +
>         dev->init_name = name;
> +       dev->release = versal_edac_release;
> +
>         rc = device_register(dev);
>         if (rc)
>                 goto err_mc_free;
>
>         mci->pdev = dev;
> -
> -       platform_set_drvdata(pdev, priv);
> -
>         mc_init(mci, dev);
> +
>         rc = edac_mc_add_mc(mci);
>         if (rc) {
>                 edac_printk(KERN_ERR, EDAC_MC, "Failed to register MC%d with EDAC
> core\n", i);
>                 goto err_unreg;
>         }
> +
> +       priv->mci[i] = mci;
> +       priv->dwidth = dt;
> +
> +       platform_set_drvdata(pdev, priv);
> +
>         return 0;
> +
>  err_unreg:
>         device_unregister(mci->pdev);
>  err_mc_free:
>         edac_mc_free(mci);
> +err_dev_free:
> +       kfree(dev);
> +err_name_free:
> +       kfree(name);
> +
>         return rc;
>  }
>
> @@ -852,10 +867,10 @@ static int init_versalnet(struct mc_priv *priv, struct
> platform_device *pdev)
>         int rc, i;
>
>         for (i = 0; i < NUM_CONTROLLERS; i++) {
> -               rc = init_mc(priv, pdev, i);
> +               rc = init_one_mc(priv, pdev, i);
>                 if (rc) {
>                         while (i--)
> -                               remove_mc(priv, i);
> +                               remove_one_mc(priv, i);
>                         return rc;
>                 }
>         }
> @@ -864,14 +879,8 @@ static int init_versalnet(struct mc_priv *priv, struct
> platform_device *pdev)
>
>  static void remove_versalnet(struct mc_priv *priv)  {
> -       struct mem_ctl_info *mci;
> -       int i;
> -
> -       for (i = 0; i < NUM_CONTROLLERS; i++) {
> -               device_unregister(priv->mci[i]->pdev);
> -               mci = edac_mc_del_mc(priv->mci[i]->pdev);
> -               edac_mc_free(mci);
> -       }
> +       for (int i = 0; i < NUM_CONTROLLERS; i++)
> +               remove_one_mc(priv, i);
>  }
>
>  static int mc_probe(struct platform_device *pdev)
>
> --
> Regards/Gruss,
>     Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v2] EDAC/versalnet: Refactor memory controller initialization and cleanup

[Borislav Petkov]

On Thu, Feb 26, 2026 at 04:50:22PM +0000, Datta, Shubhrajyoti wrote:
> Tested on the hardware.

I don't know what is going on with the communication between you and me.
Somehow there's a misunderstanding.

I asked you:

"just take your patch + my changes and do a one patch, test it and send it out
so that I can apply it"

Which part of that above is not clear?

Tell me so that I can communicate my thoughts better to you in the future.

Anyway, I did it myself again, because, oh well... :-(

Please review and test that properly and lemme know.

I hope at least this request is clear now... lemme know if not.

Thx.

---
From 103298da3e376e471b0165f006c554c93f6aee64 Mon Sep 17 00:00:00 2001
From: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
Date: Tue, 4 Nov 2025 15:09:20 +0530
Subject: [PATCH] EDAC/versalnet: Refactor memory controller initialization and
 cleanup

Simplify the initialization and cleanup flow for Versal Net DDRMC
controllers in the EDAC driver by carving out the single controller init
into a separate function which allows for a much better and more
readable error handling and unwinding.

  [ bp:
	- do the kzalloc allocations first
	- "publish" the structures only after they've been initialized
	  properly so that you don't need to unwind unnecessarily when
	  it fails later
	- remove_versalnet() is now trivial
   ]

Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
Link: https://patch.msgid.link/20251104093932.3838876-1-shubhrajyoti.datta@amd.com
---
 drivers/edac/versalnet_edac.c | 174 +++++++++++++++++++---------------
 1 file changed, 97 insertions(+), 77 deletions(-)

diff --git a/drivers/edac/versalnet_edac.c b/drivers/edac/versalnet_edac.c
index 2cbc13d9bd00..0b47ed7fed63 100644
--- a/drivers/edac/versalnet_edac.c
+++ b/drivers/edac/versalnet_edac.c
@@ -70,6 +70,8 @@
 #define XDDR5_BUS_WIDTH_32		1
 #define XDDR5_BUS_WIDTH_16		2
 
+#define MC_NAME_LEN			32
+
 /**
  * struct ecc_error_info - ECC error log information.
  * @burstpos:		Burst position.
@@ -760,7 +762,17 @@ static void versal_edac_release(struct device *dev)
 	kfree(dev);
 }
 
-static int init_versalnet(struct mc_priv *priv, struct platform_device *pdev)
+static void remove_one_mc(struct mc_priv *priv, int i)
+{
+	struct mem_ctl_info *mci;
+
+	mci = priv->mci[i];
+	device_unregister(mci->pdev);
+	edac_mc_del_mc(mci->pdev);
+	edac_mc_free(mci);
+}
+
+static int init_one_mc(struct mc_priv *priv, struct platform_device *pdev, int i)
 {
 	u32 num_chans, rank, dwidth, config;
 	struct edac_mc_layer layers[2];
@@ -768,102 +780,110 @@ static int init_versalnet(struct mc_priv *priv, struct platform_device *pdev)
 	struct device *dev;
 	enum dev_type dt;
 	char *name;
-	int rc, i;
-
-	for (i = 0; i < NUM_CONTROLLERS; i++) {
-		config = priv->adec[CONF + i * ADEC_NUM];
-		num_chans = FIELD_GET(MC5_NUM_CHANS_MASK, config);
-		rank = 1 << FIELD_GET(MC5_RANK_MASK, config);
-		dwidth = FIELD_GET(MC5_BUS_WIDTH_MASK, config);
-
-		switch (dwidth) {
-		case XDDR5_BUS_WIDTH_16:
-			dt = DEV_X16;
-			break;
-		case XDDR5_BUS_WIDTH_32:
-			dt = DEV_X32;
-			break;
-		case XDDR5_BUS_WIDTH_64:
-			dt = DEV_X64;
-			break;
-		default:
-			dt = DEV_UNKNOWN;
-		}
+	int rc;
 
-		if (dt == DEV_UNKNOWN)
-			continue;
+	config = priv->adec[CONF + i * ADEC_NUM];
+	num_chans = FIELD_GET(MC5_NUM_CHANS_MASK, config);
+	rank = 1 << FIELD_GET(MC5_RANK_MASK, config);
+	dwidth = FIELD_GET(MC5_BUS_WIDTH_MASK, config);
+
+	switch (dwidth) {
+	case XDDR5_BUS_WIDTH_16:
+		dt = DEV_X16;
+		break;
+	case XDDR5_BUS_WIDTH_32:
+		dt = DEV_X32;
+		break;
+	case XDDR5_BUS_WIDTH_64:
+		dt = DEV_X64;
+		break;
+	default:
+		dt = DEV_UNKNOWN;
+	}
 
-		/* Find the first enabled device and register that one. */
-		layers[0].type = EDAC_MC_LAYER_CHIP_SELECT;
-		layers[0].size = rank;
-		layers[0].is_virt_csrow = true;
-		layers[1].type = EDAC_MC_LAYER_CHANNEL;
-		layers[1].size = num_chans;
-		layers[1].is_virt_csrow = false;
+	if (dt == DEV_UNKNOWN)
+		return 0;
 
-		rc = -ENOMEM;
-		mci = edac_mc_alloc(i, ARRAY_SIZE(layers), layers,
-				    sizeof(struct mc_priv));
-		if (!mci) {
-			edac_printk(KERN_ERR, EDAC_MC, "Failed memory allocation for MC%d\n", i);
-			goto err_alloc;
-		}
+	/* Find the first enabled device and register that one. */
+	layers[0].type = EDAC_MC_LAYER_CHIP_SELECT;
+	layers[0].size = rank;
+	layers[0].is_virt_csrow = true;
+	layers[1].type = EDAC_MC_LAYER_CHANNEL;
+	layers[1].size = num_chans;
+	layers[1].is_virt_csrow = false;
+
+	rc = -ENOMEM;
+	name = kzalloc(MC_NAME_LEN, GFP_KERNEL);
+	if (!name)
+		return rc;
+
+	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+	if (!dev)
+		goto err_name_free;
+
+	mci = edac_mc_alloc(i, ARRAY_SIZE(layers), layers, sizeof(struct mc_priv));
+	if (!mci) {
+		edac_printk(KERN_ERR, EDAC_MC, "Failed memory allocation for MC%d\n", i);
+		goto err_dev_free;
+	}
 
-		priv->mci[i] = mci;
-		priv->dwidth = dt;
+	sprintf(name, "versal-net-ddrmc5-edac-%d", i);
 
-		dev = kzalloc_obj(*dev);
-		dev->release = versal_edac_release;
-		name = kmalloc(32, GFP_KERNEL);
-		sprintf(name, "versal-net-ddrmc5-edac-%d", i);
-		dev->init_name = name;
-		rc = device_register(dev);
-		if (rc)
-			goto err_alloc;
+	dev->init_name = name;
+	dev->release = versal_edac_release;
 
-		mci->pdev = dev;
+	rc = device_register(dev);
+	if (rc)
+		goto err_mc_free;
 
-		platform_set_drvdata(pdev, priv);
+	mci->pdev = dev;
+	mc_init(mci, dev);
 
-		mc_init(mci, dev);
-		rc = edac_mc_add_mc(mci);
-		if (rc) {
-			edac_printk(KERN_ERR, EDAC_MC, "Failed to register MC%d with EDAC core\n", i);
-			goto err_alloc;
-		}
+	rc = edac_mc_add_mc(mci);
+	if (rc) {
+		edac_printk(KERN_ERR, EDAC_MC, "Failed to register MC%d with EDAC core\n", i);
+		goto err_unreg;
 	}
-	return 0;
 
-err_alloc:
-	while (i--) {
-		mci = priv->mci[i];
-		if (!mci)
-			continue;
-
-		if (mci->pdev) {
-			device_unregister(mci->pdev);
-			edac_mc_del_mc(mci->pdev);
-		}
+	priv->mci[i] = mci;
+	priv->dwidth = dt;
 
-		edac_mc_free(mci);
-	}
+	platform_set_drvdata(pdev, priv);
+
+	return 0;
+
+err_unreg:
+	device_unregister(mci->pdev);
+err_mc_free:
+	edac_mc_free(mci);
+err_dev_free:
+	kfree(dev);
+err_name_free:
+	kfree(name);
 
 	return rc;
 }
 
-static void remove_versalnet(struct mc_priv *priv)
+static int init_versalnet(struct mc_priv *priv, struct platform_device *pdev)
 {
-	struct mem_ctl_info *mci;
-	int i;
+	int rc, i;
 
 	for (i = 0; i < NUM_CONTROLLERS; i++) {
-		device_unregister(priv->mci[i]->pdev);
-		mci = edac_mc_del_mc(priv->mci[i]->pdev);
-		if (!mci)
-			return;
+		rc = init_one_mc(priv, pdev, i);
+		if (rc) {
+			while (i--)
+				remove_one_mc(priv, i);
 
-		edac_mc_free(mci);
+			return rc;
+		}
 	}
+	return 0;
+}
+
+static void remove_versalnet(struct mc_priv *priv)
+{
+	for (int i = 0; i < NUM_CONTROLLERS; i++)
+		remove_one_mc(priv, i);
 }
 
 static int mc_probe(struct platform_device *pdev)
-- 
2.51.0

-- 
Regards/Gruss,
    Boris.

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

RE: [PATCH v2] EDAC/versalnet: Refactor memory controller initialization and cleanup

[Datta, Shubhrajyoti]

[AMD Official Use Only - AMD Internal Distribution Only]

> -----Original Message-----
> From: Borislav Petkov <bp@alien8.de>
> Sent: Saturday, February 28, 2026 2:40 PM
> To: Datta, Shubhrajyoti <shubhrajyoti.datta@amd.com>
> Cc: linux-kernel@vger.kernel.org; linux-edac@vger.kernel.org;
> shubhrajyoti.datta@gmail.com; Tony Luck <tony.luck@intel.com>; James Morse
> <james.morse@arm.com>; Mauro Carvalho Chehab <mchehab@kernel.org>;
> Robert Richter <rric@kernel.org>
> Subject: Re: [PATCH v2] EDAC/versalnet: Refactor memory controller
> initialization and cleanup
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> On Thu, Feb 26, 2026 at 04:50:22PM +0000, Datta, Shubhrajyoti wrote:
> > Tested on the hardware.
>
> I don't know what is going on with the communication between you and me.
> Somehow there's a misunderstanding.
>
> I asked you:
>
> "just take your patch + my changes and do a one patch, test it and send it out so
> that I can apply it"
>
> Which part of that above is not clear?
>
> Tell me so that I can communicate my thoughts better to you in the future.
>
> Anyway, I did it myself again, because, oh well... :-(
>
> Please review and test that properly and lemme know.
>
> I hope at least this request is clear now... lemme know if not.
>

My bad I missed the posting of the patch.

I have tested the patch below .
> Thx.
>
> ---
> From 103298da3e376e471b0165f006c554c93f6aee64 Mon Sep 17 00:00:00
> 2001
> From: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
> Date: Tue, 4 Nov 2025 15:09:20 +0530
> Subject: [PATCH] EDAC/versalnet: Refactor memory controller initialization and
> cleanup
>
> Simplify the initialization and cleanup flow for Versal Net DDRMC controllers in
> the EDAC driver by carving out the single controller init into a separate function
> which allows for a much better and more readable error handling and
> unwinding.
>
>   [ bp:
>         - do the kzalloc allocations first
>         - "publish" the structures only after they've been initialized
>           properly so that you don't need to unwind unnecessarily when
>           it fails later
>         - remove_versalnet() is now trivial
>    ]
>
> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
> Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
> Link: https://patch.msgid.link/20251104093932.3838876-1-
> shubhrajyoti.datta@amd.com
> ---
>  drivers/edac/versalnet_edac.c | 174 +++++++++++++++++++---------------
>  1 file changed, 97 insertions(+), 77 deletions(-)
>
> diff --git a/drivers/edac/versalnet_edac.c b/drivers/edac/versalnet_edac.c index
> 2cbc13d9bd00..0b47ed7fed63 100644
> --- a/drivers/edac/versalnet_edac.c
> +++ b/drivers/edac/versalnet_edac.c
> @@ -70,6 +70,8 @@
>  #define XDDR5_BUS_WIDTH_32             1
>  #define XDDR5_BUS_WIDTH_16             2
>
> +#define MC_NAME_LEN                    32
> +
>  /**
>   * struct ecc_error_info - ECC error log information.
>   * @burstpos:          Burst position.
> @@ -760,7 +762,17 @@ static void versal_edac_release(struct device *dev)
>         kfree(dev);
>  }
>
> -static int init_versalnet(struct mc_priv *priv, struct platform_device *pdev)
> +static void remove_one_mc(struct mc_priv *priv, int i) {
> +       struct mem_ctl_info *mci;
> +
> +       mci = priv->mci[i];
> +       device_unregister(mci->pdev);
> +       edac_mc_del_mc(mci->pdev);
> +       edac_mc_free(mci);
> +}
> +
> +static int init_one_mc(struct mc_priv *priv, struct platform_device
> +*pdev, int i)
>  {
>         u32 num_chans, rank, dwidth, config;
>         struct edac_mc_layer layers[2];
> @@ -768,102 +780,110 @@ static int init_versalnet(struct mc_priv *priv, struct
> platform_device *pdev)
>         struct device *dev;
>         enum dev_type dt;
>         char *name;
> -       int rc, i;
> -
> -       for (i = 0; i < NUM_CONTROLLERS; i++) {
> -               config = priv->adec[CONF + i * ADEC_NUM];
> -               num_chans = FIELD_GET(MC5_NUM_CHANS_MASK, config);
> -               rank = 1 << FIELD_GET(MC5_RANK_MASK, config);
> -               dwidth = FIELD_GET(MC5_BUS_WIDTH_MASK, config);
> -
> -               switch (dwidth) {
> -               case XDDR5_BUS_WIDTH_16:
> -                       dt = DEV_X16;
> -                       break;
> -               case XDDR5_BUS_WIDTH_32:
> -                       dt = DEV_X32;
> -                       break;
> -               case XDDR5_BUS_WIDTH_64:
> -                       dt = DEV_X64;
> -                       break;
> -               default:
> -                       dt = DEV_UNKNOWN;
> -               }
> +       int rc;
>
> -               if (dt == DEV_UNKNOWN)
> -                       continue;
> +       config = priv->adec[CONF + i * ADEC_NUM];
> +       num_chans = FIELD_GET(MC5_NUM_CHANS_MASK, config);
> +       rank = 1 << FIELD_GET(MC5_RANK_MASK, config);
> +       dwidth = FIELD_GET(MC5_BUS_WIDTH_MASK, config);
> +
> +       switch (dwidth) {
> +       case XDDR5_BUS_WIDTH_16:
> +               dt = DEV_X16;
> +               break;
> +       case XDDR5_BUS_WIDTH_32:
> +               dt = DEV_X32;
> +               break;
> +       case XDDR5_BUS_WIDTH_64:
> +               dt = DEV_X64;
> +               break;
> +       default:
> +               dt = DEV_UNKNOWN;
> +       }
>
> -               /* Find the first enabled device and register that one. */
> -               layers[0].type = EDAC_MC_LAYER_CHIP_SELECT;
> -               layers[0].size = rank;
> -               layers[0].is_virt_csrow = true;
> -               layers[1].type = EDAC_MC_LAYER_CHANNEL;
> -               layers[1].size = num_chans;
> -               layers[1].is_virt_csrow = false;
> +       if (dt == DEV_UNKNOWN)
> +               return 0;
>
> -               rc = -ENOMEM;
> -               mci = edac_mc_alloc(i, ARRAY_SIZE(layers), layers,
> -                                   sizeof(struct mc_priv));
> -               if (!mci) {
> -                       edac_printk(KERN_ERR, EDAC_MC, "Failed memory allocation for
> MC%d\n", i);
> -                       goto err_alloc;
> -               }
> +       /* Find the first enabled device and register that one. */
> +       layers[0].type = EDAC_MC_LAYER_CHIP_SELECT;
> +       layers[0].size = rank;
> +       layers[0].is_virt_csrow = true;
> +       layers[1].type = EDAC_MC_LAYER_CHANNEL;
> +       layers[1].size = num_chans;
> +       layers[1].is_virt_csrow = false;
> +
> +       rc = -ENOMEM;
> +       name = kzalloc(MC_NAME_LEN, GFP_KERNEL);
> +       if (!name)
> +               return rc;
> +
> +       dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> +       if (!dev)
> +               goto err_name_free;
> +
> +       mci = edac_mc_alloc(i, ARRAY_SIZE(layers), layers, sizeof(struct mc_priv));
> +       if (!mci) {
> +               edac_printk(KERN_ERR, EDAC_MC, "Failed memory allocation for
> MC%d\n", i);
> +               goto err_dev_free;
> +       }
>
> -               priv->mci[i] = mci;
> -               priv->dwidth = dt;
> +       sprintf(name, "versal-net-ddrmc5-edac-%d", i);
>
> -               dev = kzalloc_obj(*dev);
> -               dev->release = versal_edac_release;
> -               name = kmalloc(32, GFP_KERNEL);
> -               sprintf(name, "versal-net-ddrmc5-edac-%d", i);
> -               dev->init_name = name;
> -               rc = device_register(dev);
> -               if (rc)
> -                       goto err_alloc;
> +       dev->init_name = name;
> +       dev->release = versal_edac_release;
>
> -               mci->pdev = dev;
> +       rc = device_register(dev);
> +       if (rc)
> +               goto err_mc_free;
>
> -               platform_set_drvdata(pdev, priv);
> +       mci->pdev = dev;
> +       mc_init(mci, dev);
>
> -               mc_init(mci, dev);
> -               rc = edac_mc_add_mc(mci);
> -               if (rc) {
> -                       edac_printk(KERN_ERR, EDAC_MC, "Failed to register MC%d with
> EDAC core\n", i);
> -                       goto err_alloc;
> -               }
> +       rc = edac_mc_add_mc(mci);
> +       if (rc) {
> +               edac_printk(KERN_ERR, EDAC_MC, "Failed to register MC%d with EDAC
> core\n", i);
> +               goto err_unreg;
>         }
> -       return 0;
>
> -err_alloc:
> -       while (i--) {
> -               mci = priv->mci[i];
> -               if (!mci)
> -                       continue;
> -
> -               if (mci->pdev) {
> -                       device_unregister(mci->pdev);
> -                       edac_mc_del_mc(mci->pdev);
> -               }
> +       priv->mci[i] = mci;
> +       priv->dwidth = dt;
>
> -               edac_mc_free(mci);
> -       }
> +       platform_set_drvdata(pdev, priv);
> +
> +       return 0;
> +
> +err_unreg:
> +       device_unregister(mci->pdev);
> +err_mc_free:
> +       edac_mc_free(mci);
> +err_dev_free:
> +       kfree(dev);
> +err_name_free:
> +       kfree(name);
>
>         return rc;
>  }
>
> -static void remove_versalnet(struct mc_priv *priv)
> +static int init_versalnet(struct mc_priv *priv, struct platform_device
> +*pdev)
>  {
> -       struct mem_ctl_info *mci;
> -       int i;
> +       int rc, i;
>
>         for (i = 0; i < NUM_CONTROLLERS; i++) {
> -               device_unregister(priv->mci[i]->pdev);
> -               mci = edac_mc_del_mc(priv->mci[i]->pdev);
> -               if (!mci)
> -                       return;
> +               rc = init_one_mc(priv, pdev, i);
> +               if (rc) {
> +                       while (i--)
> +                               remove_one_mc(priv, i);
>
> -               edac_mc_free(mci);
> +                       return rc;
> +               }
>         }
> +       return 0;
> +}
> +
> +static void remove_versalnet(struct mc_priv *priv) {
> +       for (int i = 0; i < NUM_CONTROLLERS; i++)
> +               remove_one_mc(priv, i);
>  }
>
>  static int mc_probe(struct platform_device *pdev)
> --
> 2.51.0
>
> --
> Regards/Gruss,
>     Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v2] EDAC/versalnet: Refactor memory controller initialization and cleanup

[Borislav Petkov]

On Mon, Mar 02, 2026 at 11:13:03AM +0000, Datta, Shubhrajyoti wrote:
> I have tested the patch below .

Thx.

-- 
Regards/Gruss,
    Boris.

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