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

diff --git a/drivers/edac/versalnet_edac.c b/drivers/edac/versalnet_edac.c
index 2cbc13d9bd00b825c9833c0ec477e9eda1a271a4..0b47ed7fed638be41602704ea69f5622ca24748b 100644 (file)
--- 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)