Messages in this thread Patch in this message | | | Date | Wed, 6 May 2020 10:45:59 +0200 | From | Robert Richter <> | Subject | Re: [PATCH v2 08/10] EDAC/ghes: Carve out MC device handling into separate functions |
| |
On 27.04.20 18:38:56, Borislav Petkov wrote: > On Wed, Apr 22, 2020 at 01:58:12PM +0200, Robert Richter wrote:
> > +static int ghes_mc_add_or_free(struct mem_ctl_info *mci, > > + struct list_head *dimm_list) > > No, I think we talked about this already. This function should be > called: > > ghes_mc_add() > > and should do one thing and one thing only in good old unix tradition: > add the MC. > > > +{ > > unsigned long flags; > > - int idx = -1; > > + int rc; > > + > > + rc = edac_mc_add_mc(mci); > > + if (rc < 0) { > > > + ghes_dimm_release(dimm_list); > > + edac_mc_free(mci); > > + return rc; > > Those last three lines should be called by the *caller* of > ghes_mc_add(), when latter returns an error value.
These direct operations are nothing a caller should deal with.
The caller does now:
mci = ghes_mc_create(...); ... /* prepare dimms */ return ghes_mc_add_or_free(...);
To shut it down we just use:
ghes_mc_free();
Pretty simple.
Now, lets look at your suggestion to put it out of the function. A caller always needs to free the mci and dimms, so we will get:
int rc; mci = ghes_mc_create(...); ... /* prepare dimms */ rc = ghes_mc_add(...); if (rc < 0) { /* free mci */ /* free dimms */ ... } return rc;
We loose the tail call and simplicity here. Note this duplicates code as there are 2 users of ghes_mc_add().
Now, the caller does not know the implementation details, so we need to provide another release function (let's call it *_release() here):
mci = ghes_mc_create(...); ... /* prepare dimms */ rc = ghes_mc_add(...); if (rc < 0) { ghes_mc_release(mci); ghes_dimm_release(dimm_list); } return rc;
Ok, now there is another function needed to release everything.
This design also impacts ghes_mc_free(). So the shutdown implementation would turn to:
struct mem_ctl_info *mci; ... mci = ghes_mc_del(); ghes_mc_release(mci); ...
I don't see any benefit. See also below for the delta of an implementation of the suggested changes.
> > > + } > > + > > + spin_lock_irqsave(&ghes_lock, flags); > > + ghes_pvt = mci->pvt_info; > > + list_splice_tail(dimm_list, &ghes_dimm_list); > > + spin_unlock_irqrestore(&ghes_lock, flags); > > + > > + return 0; > > +} > > + > > +static void ghes_mc_free(void) > > +{ > > + struct mem_ctl_info *mci; > > + unsigned long flags; > > + LIST_HEAD(dimm_list); > > + > > + /* > > + * Wait for the irq handler being finished. > > + */ > > + spin_lock_irqsave(&ghes_lock, flags); > > + mci = ghes_pvt ? ghes_pvt->mci : NULL; > > + ghes_pvt = NULL; > > + list_splice_init(&ghes_dimm_list, &dimm_list); > > + spin_unlock_irqrestore(&ghes_lock, flags); > > + > > + ghes_dimm_release(&dimm_list); > > + > > + if (!mci) > > + return; > > + > > + mci = edac_mc_del_mc(mci->pdev); > > + if (mci) > > + edac_mc_free(mci); > > +} > > This function needs to do only freeing of the mc. The list splicing and > dimm releasing needs to be done by its caller, before calling it.
ghes_mc_free() is the counterpart to ghes_mc_add() and thus needs to also handle the dimm_list here. This cannot be left to the caller.
Considering all the above, I don't see how your suggestions to the interface could improve the code. Hmm...
Below an implementation that illustrates the changes.
Thanks,
-Robert
--- drivers/edac/ghes_edac.c | 41 +++++++++++++++++++++++++++------------- 1 file changed, 28 insertions(+), 13 deletions(-)
diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c index 7f39346d895b..896d7b488fc2 100644 --- a/drivers/edac/ghes_edac.c +++ b/drivers/edac/ghes_edac.c @@ -576,18 +576,14 @@ static struct mem_ctl_info *ghes_mc_create(struct device *dev, int mc_idx, return mci; } -static int ghes_mc_add_or_free(struct mem_ctl_info *mci, - struct list_head *dimms) +static int ghes_mc_add(struct mem_ctl_info *mci, struct list_head *dimms) { unsigned long flags; int rc; rc = edac_mc_add_mc(mci); - if (rc < 0) { - dimm_release(dimms); - edac_mc_free(mci); + if (rc < 0) return rc; - } spin_lock_irqsave(&ghes_lock, flags); ghes_pvt = mci->pvt_info; @@ -597,7 +593,7 @@ static int ghes_mc_add_or_free(struct mem_ctl_info *mci, return 0; } -static void ghes_mc_free(void) +static struct mem_ctl_info *ghes_mc_del(void) { struct mem_ctl_info *mci; unsigned long flags; @@ -614,10 +610,14 @@ static void ghes_mc_free(void) dimm_release(&dimms); - if (!mci) - return; + if (mci) + mci = edac_mc_del_mc(mci->pdev); - mci = edac_mc_del_mc(mci->pdev); + return mci; +} + +static void ghes_mc_release(struct mem_ctl_info *mci) +{ if (mci) edac_mc_free(mci); } @@ -627,6 +627,7 @@ static int ghes_edac_register_fake(struct device *dev) struct mem_ctl_info *mci; struct dimm_info *dimm; LIST_HEAD(empty); + int rc; mci = ghes_mc_create(dev, 0, 1); if (!mci) @@ -642,13 +643,18 @@ static int ghes_edac_register_fake(struct device *dev) snprintf(dimm->label, sizeof(dimm->label), "unknown memory"); - return ghes_mc_add_or_free(mci, &empty); + rc = ghes_mc_add(mci, &empty); + if (rc < 0) + ghes_mc_free(mci); + + return rc; } static int ghes_edac_register_one(struct device *dev, int mc_idx, int num_dimm) { struct dimm_fill dimm_fill; struct mem_ctl_info *mci; + int rc; mci = ghes_mc_create(dev, mc_idx, num_dimm); if (!mci) @@ -660,7 +666,13 @@ static int ghes_edac_register_one(struct device *dev, int mc_idx, int num_dimm) dmi_walk(ghes_edac_dmidecode, &dimm_fill); - return ghes_mc_add_or_free(mci, &dimm_fill.dimms); + rc = ghes_mc_add(mci, &dimm_fill.dimms); + if (rc < 0) { + dimm_release(&dimm_fill.dimms); + ghes_mc_release(mci); + } + + return rc; } int ghes_edac_register(struct ghes *ghes, struct device *dev) @@ -740,10 +752,13 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev) void ghes_edac_unregister(struct ghes *ghes) { + struct mem_ctl_info *mci; + mutex_lock(&ghes_reg_mutex); if (refcount_dec_and_test(&ghes_refcount)) { - ghes_mc_free(); + mci = ghes_mc_del(); + ghes_mc_release(mci); dimm_pool_destroy(); } -- 2.20.1
| |