diff mbox series

[06/11] EDAC/ghes: Carve out MC device handling into separate functions

Message ID 20200306151318.17422-7-rrichter@marvell.com (mailing list archive)
State New, archived
Headers show
Series EDAC/ghes: Cleanup, rework and improvement of memory reporting | expand

Commit Message

Robert Richter March 6, 2020, 3:13 p.m. UTC 
The functions are too long, carve out code that handles MC devices
into the new functions ghes_mc_create(), ghes_mc_add_or_free() and
ghes_mc_free(). Apart from better code readability the functions can
be reused and the implementation of the error paths becomes easier.

Signed-off-by: Robert Richter <rrichter@marvell.com>
---
 drivers/edac/ghes_edac.c | 133 +++++++++++++++++++++++----------------
 1 file changed, 79 insertions(+), 54 deletions(-)

Comments

Borislav Petkov March 16, 2020, 9:31 a.m. UTC | #1 
On Fri, Mar 06, 2020 at 04:13:13PM +0100, Robert Richter wrote:
> The functions are too long, carve out code that handles MC devices
> into the new functions ghes_mc_create(), ghes_mc_add_or_free() and
> ghes_mc_free(). Apart from better code readability the functions can
> be reused and the implementation of the error paths becomes easier.
> 
> Signed-off-by: Robert Richter <rrichter@marvell.com>
> ---
>  drivers/edac/ghes_edac.c | 133 +++++++++++++++++++++++----------------
>  1 file changed, 79 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
> index 358519e8c2e9..5a4c9694bbff 100644
> --- a/drivers/edac/ghes_edac.c
> +++ b/drivers/edac/ghes_edac.c
> @@ -462,16 +462,81 @@ static struct acpi_platform_list plat_list[] = {
>  	{ } /* End */
>  };
>  
> -int ghes_edac_register(struct ghes *ghes, struct device *dev)
> +static struct mem_ctl_info *ghes_mc_create(struct device *dev, int mc_idx,
> +					int num_dimm)
>  {
> -	bool fake = false;
> -	int rc = 0, num_dimm = 0;
> +	struct edac_mc_layer layers[1];
>  	struct mem_ctl_info *mci;
>  	struct ghes_mci *pvt;
> -	struct edac_mc_layer layers[1];
> -	struct ghes_dimm_fill dimm_fill;
> +
> +	layers[0].type = EDAC_MC_LAYER_ALL_MEM;
> +	layers[0].size = num_dimm;
> +	layers[0].is_virt_csrow = true;
> +
> +	mci = edac_mc_alloc(mc_idx, ARRAY_SIZE(layers), layers, sizeof(*pvt));
> +	if (!mci)
> +		return NULL;
> +
> +	pvt		= mci->pvt_info;
> +	pvt->mci	= mci;
> +
> +	mci->pdev = dev;
> +	mci->mtype_cap = MEM_FLAG_EMPTY;
> +	mci->edac_ctl_cap = EDAC_FLAG_NONE;
> +	mci->edac_cap = EDAC_FLAG_NONE;
> +	mci->mod_name = "ghes_edac.c";
> +	mci->ctl_name = "ghes_edac";
> +	mci->dev_name = "ghes";
> +
> +	return mci;
> +}
> +
> +static int ghes_mc_add_or_free(struct mem_ctl_info *mci)

ghes_mc_add() is good enough. The fact that the function has error
handling doesn't need to be in the name.
Robert Richter March 16, 2020, 12:12 p.m. UTC | #2 
On 16.03.20 10:31:34, Borislav Petkov wrote:
> On Fri, Mar 06, 2020 at 04:13:13PM +0100, Robert Richter wrote:
> > The functions are too long, carve out code that handles MC devices
> > into the new functions ghes_mc_create(), ghes_mc_add_or_free() and
> > ghes_mc_free(). Apart from better code readability the functions can
> > be reused and the implementation of the error paths becomes easier.
> > 
> > Signed-off-by: Robert Richter <rrichter@marvell.com>
> > ---
> >  drivers/edac/ghes_edac.c | 133 +++++++++++++++++++++++----------------
> >  1 file changed, 79 insertions(+), 54 deletions(-)
> > 
> > diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
> > index 358519e8c2e9..5a4c9694bbff 100644
> > --- a/drivers/edac/ghes_edac.c
> > +++ b/drivers/edac/ghes_edac.c
> > @@ -462,16 +462,81 @@ static struct acpi_platform_list plat_list[] = {
> >  	{ } /* End */
> >  };
> >  
> > -int ghes_edac_register(struct ghes *ghes, struct device *dev)
> > +static struct mem_ctl_info *ghes_mc_create(struct device *dev, int mc_idx,
> > +					int num_dimm)
> >  {
> > -	bool fake = false;
> > -	int rc = 0, num_dimm = 0;
> > +	struct edac_mc_layer layers[1];
> >  	struct mem_ctl_info *mci;
> >  	struct ghes_mci *pvt;
> > -	struct edac_mc_layer layers[1];
> > -	struct ghes_dimm_fill dimm_fill;
> > +
> > +	layers[0].type = EDAC_MC_LAYER_ALL_MEM;
> > +	layers[0].size = num_dimm;
> > +	layers[0].is_virt_csrow = true;
> > +
> > +	mci = edac_mc_alloc(mc_idx, ARRAY_SIZE(layers), layers, sizeof(*pvt));
> > +	if (!mci)
> > +		return NULL;
> > +
> > +	pvt		= mci->pvt_info;
> > +	pvt->mci	= mci;
> > +
> > +	mci->pdev = dev;
> > +	mci->mtype_cap = MEM_FLAG_EMPTY;
> > +	mci->edac_ctl_cap = EDAC_FLAG_NONE;
> > +	mci->edac_cap = EDAC_FLAG_NONE;
> > +	mci->mod_name = "ghes_edac.c";
> > +	mci->ctl_name = "ghes_edac";
> > +	mci->dev_name = "ghes";
> > +
> > +	return mci;
> > +}
> > +
> > +static int ghes_mc_add_or_free(struct mem_ctl_info *mci)
> 
> ghes_mc_add() is good enough. The fact that the function has error
> handling doesn't need to be in the name.

It's not just error handling here. I choose the name as the mci is
freed if it could not be added, which is different to just return an
error if it fails. Otherwise the flow would be something like:

	mci = ghes_mc_create(...);
	rc = ghes_mc_add(mci);
	if (rc) {
		ghes_mc_free(mci);
		/* do something */
	}

But we do not need mci any longer on error, so we can free it directly
which makes the code much easier, but in fact it does 2 things at a
time then:

	mci = ghes_mc_create(...);
	rc = ghes_mc_add_or_free(mci);
	if (rc)
        	/* do something */

I would rather prefer ghes_mc_add_or_free().

-Robert
diff mbox series

Patch

diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
index 358519e8c2e9..5a4c9694bbff 100644
--- a/drivers/edac/ghes_edac.c
+++ b/drivers/edac/ghes_edac.c
@@ -462,16 +462,81 @@  static struct acpi_platform_list plat_list[] = {
 	{ } /* End */
 };
 
-int ghes_edac_register(struct ghes *ghes, struct device *dev)
+static struct mem_ctl_info *ghes_mc_create(struct device *dev, int mc_idx,
+					int num_dimm)
 {
-	bool fake = false;
-	int rc = 0, num_dimm = 0;
+	struct edac_mc_layer layers[1];
 	struct mem_ctl_info *mci;
 	struct ghes_mci *pvt;
-	struct edac_mc_layer layers[1];
-	struct ghes_dimm_fill dimm_fill;
+
+	layers[0].type = EDAC_MC_LAYER_ALL_MEM;
+	layers[0].size = num_dimm;
+	layers[0].is_virt_csrow = true;
+
+	mci = edac_mc_alloc(mc_idx, ARRAY_SIZE(layers), layers, sizeof(*pvt));
+	if (!mci)
+		return NULL;
+
+	pvt		= mci->pvt_info;
+	pvt->mci	= mci;
+
+	mci->pdev = dev;
+	mci->mtype_cap = MEM_FLAG_EMPTY;
+	mci->edac_ctl_cap = EDAC_FLAG_NONE;
+	mci->edac_cap = EDAC_FLAG_NONE;
+	mci->mod_name = "ghes_edac.c";
+	mci->ctl_name = "ghes_edac";
+	mci->dev_name = "ghes";
+
+	return mci;
+}
+
+static int ghes_mc_add_or_free(struct mem_ctl_info *mci)
+{
 	unsigned long flags;
-	int idx = -1;
+	int rc;
+
+	rc = edac_mc_add_mc(mci);
+	if (rc < 0) {
+		edac_mc_free(mci);
+		return rc;
+	}
+
+	spin_lock_irqsave(&ghes_lock, flags);
+	ghes_pvt = mci->pvt_info;
+	spin_unlock_irqrestore(&ghes_lock, flags);
+
+	return 0;
+}
+
+static void ghes_mc_free(void)
+{
+	struct mem_ctl_info *mci;
+	unsigned long flags;
+
+	/*
+	 * Wait for the irq handler being finished.
+	 */
+	spin_lock_irqsave(&ghes_lock, flags);
+	mci = ghes_pvt ? ghes_pvt->mci : NULL;
+	ghes_pvt = NULL;
+	spin_unlock_irqrestore(&ghes_lock, flags);
+
+	if (!mci)
+		return;
+
+	mci = edac_mc_del_mc(mci->pdev);
+	if (mci)
+		edac_mc_free(mci);
+}
+
+int ghes_edac_register(struct ghes *ghes, struct device *dev)
+{
+	struct ghes_dimm_fill dimm_fill;
+	int rc = 0, num_dimm = 0;
+	struct mem_ctl_info *mci;
+	bool fake = false;
+	int idx;
 
 	if (IS_ENABLED(CONFIG_X86)) {
 		/* Check if safe to enable on this system */
@@ -500,28 +565,12 @@  int ghes_edac_register(struct ghes *ghes, struct device *dev)
 		num_dimm = 1;
 	}
 
-	layers[0].type = EDAC_MC_LAYER_ALL_MEM;
-	layers[0].size = num_dimm;
-	layers[0].is_virt_csrow = true;
-
-	mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers, sizeof(*pvt));
+	mci = ghes_mc_create(dev, 0, num_dimm);
 	if (!mci) {
-		pr_info("Can't allocate memory for EDAC data\n");
 		rc = -ENOMEM;
 		goto unlock;
 	}
 
-	pvt		= mci->pvt_info;
-	pvt->mci	= mci;
-
-	mci->pdev = dev;
-	mci->mtype_cap = MEM_FLAG_EMPTY;
-	mci->edac_ctl_cap = EDAC_FLAG_NONE;
-	mci->edac_cap = EDAC_FLAG_NONE;
-	mci->mod_name = "ghes_edac.c";
-	mci->ctl_name = "ghes_edac";
-	mci->dev_name = "ghes";
-
 	if (fake) {
 		pr_info("This system has a very crappy BIOS: It doesn't even list the DIMMS.\n");
 		pr_info("Its SMBIOS info is wrong. It is doubtful that the error report would\n");
@@ -549,22 +598,17 @@  int ghes_edac_register(struct ghes *ghes, struct device *dev)
 		dimm->edac_mode = EDAC_SECDED;
 	}
 
-	rc = edac_mc_add_mc(mci);
-	if (rc < 0) {
-		pr_info("Can't register at EDAC core\n");
-		edac_mc_free(mci);
-		rc = -ENODEV;
+	rc = ghes_mc_add_or_free(mci);
+	if (rc)
 		goto unlock;
-	}
-
-	spin_lock_irqsave(&ghes_lock, flags);
-	ghes_pvt = pvt;
-	spin_unlock_irqrestore(&ghes_lock, flags);
 
 	/* only set on success */
 	refcount_set(&ghes_refcount, 1);
 
 unlock:
+	if (rc)
+		pr_err("Can't register at EDAC core: %d\n", rc);
+
 	mutex_unlock(&ghes_reg_mutex);
 
 	return rc;
@@ -572,29 +616,10 @@  int ghes_edac_register(struct ghes *ghes, struct device *dev)
 
 void ghes_edac_unregister(struct ghes *ghes)
 {
-	struct mem_ctl_info *mci;
-	unsigned long flags;
-
 	mutex_lock(&ghes_reg_mutex);
 
-	if (!refcount_dec_and_test(&ghes_refcount))
-		goto unlock;
-
-	/*
-	 * Wait for the irq handler being finished.
-	 */
-	spin_lock_irqsave(&ghes_lock, flags);
-	mci = ghes_pvt ? ghes_pvt->mci : NULL;
-	ghes_pvt = NULL;
-	spin_unlock_irqrestore(&ghes_lock, flags);
-
-	if (!mci)
-		goto unlock;
-
-	mci = edac_mc_del_mc(mci->pdev);
-	if (mci)
-		edac_mc_free(mci);
+	if (refcount_dec_and_test(&ghes_refcount))
+		ghes_mc_free();
 
-unlock:
 	mutex_unlock(&ghes_reg_mutex);
 }