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@xxxxxxxxx>
> Sent: Saturday, February 28, 2026 2:40 PM
> To: Datta, Shubhrajyoti <shubhrajyoti.datta@xxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx; linux-edac@xxxxxxxxxxxxxxx;
> shubhrajyoti.datta@xxxxxxxxx; Tony Luck <tony.luck@xxxxxxxxx>; James Morse
> <james.morse@xxxxxxx>; Mauro Carvalho Chehab <mchehab@xxxxxxxxxx>;
> Robert Richter <rric@xxxxxxxxxx>
> 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@xxxxxxx>
> 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@xxxxxxx>
> Signed-off-by: Borislav Petkov (AMD) <bp@xxxxxxxxx>
> Link: https://patch.msgid.link/20251104093932.3838876-1-
> shubhrajyoti.datta@xxxxxxx
> ---
>  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