* [PATCH v2] EDAC/versalnet: Refactor memory controller initialization and cleanup @ 2025-11-04 9:39 Shubhrajyoti Datta 2025-11-09 15:58 ` Borislav Petkov 0 siblings, 1 reply; 6+ messages in thread From: Shubhrajyoti Datta @ 2025-11-04 9:39 UTC (permalink / raw) To: linux-kernel, linux-edac Cc: shubhrajyoti.datta, Borislav Petkov, Tony Luck, James Morse, Mauro Carvalho Chehab, Robert Richter, 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] EDAC/versalnet: Refactor memory controller initialization and cleanup 2025-11-04 9:39 [PATCH v2] EDAC/versalnet: Refactor memory controller initialization and cleanup Shubhrajyoti Datta @ 2025-11-09 15:58 ` Borislav Petkov 2026-02-26 16:50 ` Datta, Shubhrajyoti 0 siblings, 1 reply; 6+ messages in thread From: Borislav Petkov @ 2025-11-09 15:58 UTC (permalink / raw) To: Shubhrajyoti Datta Cc: linux-kernel, linux-edac, shubhrajyoti.datta, Tony Luck, James Morse, Mauro Carvalho Chehab, Robert Richter 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* RE: [PATCH v2] EDAC/versalnet: Refactor memory controller initialization and cleanup 2025-11-09 15:58 ` Borislav Petkov @ 2026-02-26 16:50 ` Datta, Shubhrajyoti 2026-02-28 9:09 ` Borislav Petkov 0 siblings, 1 reply; 6+ messages in thread From: Datta, Shubhrajyoti @ 2026-02-26 16:50 UTC (permalink / raw) To: Borislav Petkov Cc: linux-kernel@vger.kernel.org, linux-edac@vger.kernel.org, shubhrajyoti.datta@gmail.com, Tony Luck, James Morse, Mauro Carvalho Chehab, Robert Richter [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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] EDAC/versalnet: Refactor memory controller initialization and cleanup 2026-02-26 16:50 ` Datta, Shubhrajyoti @ 2026-02-28 9:09 ` Borislav Petkov 2026-03-02 11:13 ` Datta, Shubhrajyoti 0 siblings, 1 reply; 6+ messages in thread From: Borislav Petkov @ 2026-02-28 9:09 UTC (permalink / raw) To: Datta, Shubhrajyoti Cc: linux-kernel@vger.kernel.org, linux-edac@vger.kernel.org, shubhrajyoti.datta@gmail.com, Tony Luck, James Morse, Mauro Carvalho Chehab, Robert Richter 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* RE: [PATCH v2] EDAC/versalnet: Refactor memory controller initialization and cleanup 2026-02-28 9:09 ` Borislav Petkov @ 2026-03-02 11:13 ` Datta, Shubhrajyoti 2026-03-02 13:24 ` Borislav Petkov 0 siblings, 1 reply; 6+ messages in thread From: Datta, Shubhrajyoti @ 2026-03-02 11:13 UTC (permalink / raw) To: Borislav Petkov Cc: linux-kernel@vger.kernel.org, linux-edac@vger.kernel.org, shubhrajyoti.datta@gmail.com, Tony Luck, James Morse, Mauro Carvalho Chehab, Robert Richter [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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] EDAC/versalnet: Refactor memory controller initialization and cleanup 2026-03-02 11:13 ` Datta, Shubhrajyoti @ 2026-03-02 13:24 ` Borislav Petkov 0 siblings, 0 replies; 6+ messages in thread From: Borislav Petkov @ 2026-03-02 13:24 UTC (permalink / raw) To: Datta, Shubhrajyoti Cc: linux-kernel@vger.kernel.org, linux-edac@vger.kernel.org, shubhrajyoti.datta@gmail.com, Tony Luck, James Morse, Mauro Carvalho Chehab, Robert Richter 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2026-03-02 13:24 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-11-04 9:39 [PATCH v2] EDAC/versalnet: Refactor memory controller initialization and cleanup Shubhrajyoti Datta 2025-11-09 15:58 ` Borislav Petkov 2026-02-26 16:50 ` Datta, Shubhrajyoti 2026-02-28 9:09 ` Borislav Petkov 2026-03-02 11:13 ` Datta, Shubhrajyoti 2026-03-02 13:24 ` Borislav Petkov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox